Support AysncSecurityPolicy in SecurityPolicies.allOf.

[mateusazis]

Addresses part of #11547.

[jdcormie]

This doesn't seem to be a valid implementation of the checkAuthorizationAsync() API contract because it blocks the calling thread.

[jdcormie]

If ALL of your input securityPolicies are AsyncSecurityPolicy, I think you can implement an async allOf(). But if even one of your inputs is not an AsyncSecurityPolicy, someone needs to provide an Executor to make this possible.

[mateusazis]

My goal was to not force the clients to pick another API if they have any AsyncSecurityPolicy, specially because the current method is still compatible (though inefficient) with AsyncSecurityPolicy, so I felt like having two methods could be confusing.

This doesn't seem to be a valid implementation of the checkAuthorizationAsync() API contract because it blocks the calling thread.

My intention was to treat all policies as AsyncSecurityPolicy by obtaining a List<ListenableFuture<Status>> and transforming them without blocking. If one of the policies is synchronous, we just produce an immediate future.

This could be harmful, yes, if that policy were slow, but I see that as API misuse: the integrator should have provided an AsyncSecurityPolicy instead and we wouldn't monopolize a thread waiting on it.

Please let me know what you think.

[jdcormie]

IDK what more to say except that implementations of checkAuthorizationAsync() aren't allowed to block. If they were, there wouldn't be much reason for AsyncSecurityPolicy to even exist: Plain old SecurityPolicy.checkAuthorization() has always had the property that it may or may not block. And we've always been able to turn checkAuthorization() into a ListenableFuture using Futures.submit(securityPolicy::checkAuthorization, blockingExecutor). What makes checkAuthorizationAsync() interesting is that it promises not to block so I can call it from any thread.

Hiding a blocking call to checkAuthorization() behind checkAuthorizationAsync() actively defeats one of your two most important callers here:

grpc-java/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java

Lines 778 to 782 in d2d8ed8

	ListenableFuture<Status> authFuture =
	(securityPolicy instanceof AsyncSecurityPolicy)
	? ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(remoteUid)
	: Futures.submit(
	() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor);

This could be harmful, yes, if that policy were slow, but I see that as API misuse: the integrator should have provided an AsyncSecurityPolicy instead and we wouldn't monopolize a thread waiting on it.

OK but your javadoc doesn't say any of that. Even if it did, we want APIs that are difficult to misuse and, ideally, impossible to misuse, by way of checks at compile time.

[mateusazis]

I see. I was approaching this with the intent that it would retroactively work for any usage of allOf without any required API changes (by requesting the addition of a Executor). I didn't feel like clients would correctly and proactively know they should use a separate method if they have 1 or more AsyncSecurityPolicy.

I see 3 options going forward:

1. Take only async policies:

   public static AsyncSecurityPolicy allOf(AsyncSecurityPolicies...policies)

   An executor may be passed for use in Futures.transform or we omit it since the computation is likely lightweight enough for the direct executor.

   Pros: minimal room for API misuse
   Cons: puts the burden on the caller to convert any synchronous SecurityPolicy into AsyncSecurityPolicy. We could mitigate this with by providing helpers for converting SecurityPolicy or Function<Integer, Status> into AsyncSecurityPolicy.

2. (current approach) Take a mix of policy types and assume correct usage

   We could improve Javadoc to discourage slow operations in the synchronous SecurityPolicy.

   Cons: room for API misuse already covered by you above

3. Take an Executor to convert the SynchronousPolicies into async:

   public static AsyncSecurityPolicy allOf(ListeningExecutorService blockingExecutor, SyncSecurityPolicies...policies) {
     List<AsyncSecurityPolicy> asyncPolicies = policies.map(policy -> {
       if (policy instanceof AsyncSecurityPolicy) {
         return policy;
       }
       return asAsyncPolicy(policy, blockingExecutor);
     }).collect(toList());
     // ...
   }

   Cons: assumes all blocking policies can be converted under the same executor; option 1 is a more generalized version of this that puts that responsibility on the caller.

Do you feel like any of these would be preferable?

[jdcormie]

It would help to know your context and use case motivating this new function. Without that though, I would lean towards your (1). The caller burden you mention as a con is just Futures.submit(securityPolicy::authorize, executor) right? The problem with (3) is that the caller must provide an Executor even if all input policies are already async.

[jdcormie]

I'm always wary of directExecutor() but I think this application would pass the tests at go/android-concurrent/directexecutor.