From 562e571a7bcddde307277945600efb5a50c8d11c Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 11 Jun 2024 12:01:16 -0700 Subject: [PATCH 01/21] Use a new channel offload executor for non-async SecurityPolicy evaluation --- .../binder/internal/BinderTransportTest.java | 1 + .../java/io/grpc/binder/BinderInternal.java | 8 +++-- .../io/grpc/binder/BinderServerBuilder.java | 19 ++++++++++ .../io/grpc/binder/ServerSecurityPolicy.java | 12 +++---- .../io/grpc/binder/internal/BinderServer.java | 35 +++++++++++++++++-- .../grpc/binder/internal/BinderTransport.java | 2 +- .../grpc/binder/ServerSecurityPolicyTest.java | 13 +++---- 7 files changed, 70 insertions(+), 20 deletions(-) diff --git a/binder/src/androidTest/java/io/grpc/binder/internal/BinderTransportTest.java b/binder/src/androidTest/java/io/grpc/binder/internal/BinderTransportTest.java index e4027b70296..6376ddabfa9 100644 --- a/binder/src/androidTest/java/io/grpc/binder/internal/BinderTransportTest.java +++ b/binder/src/androidTest/java/io/grpc/binder/internal/BinderTransportTest.java @@ -72,6 +72,7 @@ protected InternalServer newServer(List streamTracer BinderServer binderServer = new BinderServer.Builder() .setListenAddress(addr) .setExecutorServicePool(executorServicePool) + .setOffloadExecutorPool(offloadExecutorPool) .setStreamTracerFactories(streamTracerFactories) .build(); diff --git a/binder/src/main/java/io/grpc/binder/BinderInternal.java b/binder/src/main/java/io/grpc/binder/BinderInternal.java index 18af43ce2b3..6d610d172f1 100644 --- a/binder/src/main/java/io/grpc/binder/BinderInternal.java +++ b/binder/src/main/java/io/grpc/binder/BinderInternal.java @@ -17,8 +17,12 @@ package io.grpc.binder; import android.os.IBinder; +import com.google.common.util.concurrent.ListenableFuture; import io.grpc.Internal; +import io.grpc.Status; import io.grpc.binder.internal.BinderTransportSecurity; +import io.grpc.binder.internal.BinderTransportSecurity.ServerPolicyChecker; +import java.util.concurrent.Executor; /** * Helper class to expose IBinderReceiver methods for legacy internal builders. @@ -39,7 +43,7 @@ public static void setIBinder(IBinderReceiver receiver, IBinder binder) { * without causing hard dependencies on a specific class. */ public static BinderTransportSecurity.ServerPolicyChecker createPolicyChecker( - ServerSecurityPolicy securityPolicy) { - return securityPolicy::checkAuthorizationForServiceAsync; + ServerSecurityPolicy securityPolicy, Executor executor) { + return (uid, serviceName) -> securityPolicy.checkAuthorizationForServiceAsync(uid, serviceName, executor); } } diff --git a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java index 52122c5e8b7..ef1f6428afb 100644 --- a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java @@ -150,6 +150,25 @@ public BinderServerBuilder useTransportSecurity(File certChain, File privateKey) throw new UnsupportedOperationException("TLS not supported in BinderServer"); } + /** + * Provides a custom executor that will be used for operations that block or are expensive, to + * avoid blocking asynchronous code paths. For example, SecurityPolicy evaluation uses this + * executor (many implementations require one or more IPC round trips to Android's system server). + * + *

It's an optional parameter. If the user has not provided an executor when the channel is + * built, the builder will use a static cached thread pool. + * + *

The channel won't take ownership of the given executor. Callers must ensure that it lives at + * least as long as the servers you build. + * + * @return this + */ + public BinderServerBuilder offloadExecutor(Executor executor) { + internalBuilder.setOffloadExecutorPool( + new FixedObjectPool<>(checkNotNull(executor, "offloadExecutor"))); + return this; + } + /** * Builds a {@link Server} according to this builder's parameters and stores its listening {@link * IBinder} in the {@link IBinderReceiver} passed to {@link #forAddress(AndroidComponentAddress, diff --git a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java index ced973ede1c..96e1dfe5d7c 100644 --- a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java +++ b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java @@ -22,6 +22,7 @@ import io.grpc.Status; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.Executor; import javax.annotation.CheckReturnValue; /** @@ -66,22 +67,17 @@ public Status checkAuthorizationForService(int uid, String serviceName) { * * @param uid The Android UID to authenticate. * @param serviceName The name of the gRPC service being called. + * @param offloadExecutor used for evaluating the relevant SecurityPolicy if not natively async * @return a future with the result of the authorization check. A failed future represents a * failure to perform the authorization check, not that the access is denied. */ @CheckReturnValue - ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName) { + ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName, Executor offloadExecutor) { SecurityPolicy securityPolicy = perServicePolicies.getOrDefault(serviceName, defaultPolicy); if (securityPolicy instanceof AsyncSecurityPolicy) { return ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(uid); } - - try { - Status status = securityPolicy.checkAuthorization(uid); - return Futures.immediateFuture(status); - } catch (Exception e) { - return Futures.immediateFailedFuture(e); - } + return Futures.submit(() -> securityPolicy.checkAuthorization(uid), offloadExecutor); } public static Builder newBuilder() { diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java index d3580dbd139..c5e7815893d 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java @@ -43,6 +43,7 @@ import java.io.IOException; import java.net.SocketAddress; import java.util.List; +import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; import java.util.logging.Level; import java.util.logging.Logger; @@ -63,28 +64,37 @@ public final class BinderServer implements InternalServer, LeakSafeOneWayBinder. private static final Logger logger = Logger.getLogger(BinderServer.class.getName()); private final ObjectPool executorServicePool; + private final ObjectPool offloadExecutorPool; private final ImmutableList streamTracerFactories; private final AndroidComponentAddress listenAddress; private final LeakSafeOneWayBinder hostServiceBinder; - private final BinderTransportSecurity.ServerPolicyChecker serverPolicyChecker; + private final ServerSecurityPolicy serverSecurityPolicy; private final InboundParcelablePolicy inboundParcelablePolicy; private final Runnable terminationListener; @GuardedBy("this") private ServerListener listener; + @GuardedBy("this") + private BinderTransportSecurity.ServerPolicyChecker serverPolicyChecker; + @GuardedBy("this") private ScheduledExecutorService executorService; + @GuardedBy("this") + private Executor offloadExecutor; + @GuardedBy("this") private boolean shutdown; private BinderServer(Builder builder) { this.listenAddress = checkNotNull(builder.listenAddress); this.executorServicePool = builder.executorServicePool; + this.offloadExecutorPool = builder.offloadExecutorPool != null ? + builder.offloadExecutorPool : builder.executorServicePool; this.streamTracerFactories = ImmutableList.copyOf(checkNotNull(builder.streamTracerFactories, "streamTracerFactories")); - this.serverPolicyChecker = BinderInternal.createPolicyChecker(builder.serverSecurityPolicy); + this.serverSecurityPolicy = builder.serverSecurityPolicy; this.inboundParcelablePolicy = builder.inboundParcelablePolicy; this.terminationListener = builder.terminationListener; hostServiceBinder = new LeakSafeOneWayBinder(this); @@ -97,8 +107,10 @@ public IBinder getHostBinder() { @Override public synchronized void start(ServerListener serverListener) throws IOException { - listener = new ActiveTransportTracker(serverListener, terminationListener); + listener = new ActiveTransportTracker(serverListener, this::onTermination); executorService = executorServicePool.getObject(); + offloadExecutor = offloadExecutorPool.getObject(); + serverPolicyChecker = BinderInternal.createPolicyChecker(serverSecurityPolicy, offloadExecutor); } @Override @@ -129,10 +141,16 @@ public synchronized void shutdown() { // Break the connection to the binder. We'll receive no more transactions. hostServiceBinder.setHandler(GoAwayHandler.INSTANCE); listener.serverShutdown(); + // TODO(jdcormie): Should this move to onTermination? Who even uses this? executorService = executorServicePool.returnObject(executorService); } } + private void onTermination() { + offloadExecutor = offloadExecutorPool.returnObject(offloadExecutor); + terminationListener.run(); + } + @Override public String toString() { return "BinderServer[" + listenAddress + "]"; @@ -202,6 +220,7 @@ public boolean handleTransaction(int code, Parcel parcel) { public static class Builder { @Nullable AndroidComponentAddress listenAddress; @Nullable List streamTracerFactories; + @Nullable ObjectPool offloadExecutorPool; ObjectPool executorServicePool = SharedResourcePool.forResource(GrpcUtil.TIMER_SERVICE); @@ -247,6 +266,16 @@ public Builder setExecutorServicePool( return this; } + /** + * Sets the executor to be used for blocking work. + * + *

Optional. If unset, 'executorServicePool' will be used for this work (not recommended). + */ + public Builder setOffloadExecutorPool(ObjectPool offloadExecutorPool) { + this.offloadExecutorPool = offloadExecutorPool; + return this; + } + /** * Sets the {@link ServerSecurityPolicy} to be used for built servers. * diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java index 9deb2bfaea1..debe6d539b7 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -653,7 +653,7 @@ private synchronized void onReadyTimeout() { } @Override - public synchronized ClientStream newStream( + public ClientStream newStream( final MethodDescriptor method, final Metadata headers, final CallOptions callOptions, diff --git a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java index 86bc4c5ae91..56042fbdfdf 100644 --- a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java +++ b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java @@ -17,6 +17,7 @@ package io.grpc.binder; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; @@ -154,7 +155,7 @@ public void testPerServiceAsync() throws Exception { // of futures gets properly handled. ListenableFuture dependency = Futures.immediateVoidFuture(); return Futures - .transform(dependency, unused -> Status.OK, MoreExecutors.directExecutor()); + .transform(dependency, unused -> Status.OK, directExecutor()); })) .build(); @@ -180,7 +181,7 @@ public void testPerService_failedSecurityPolicyFuture_returnsAFailedFuture() { .build(); ListenableFuture statusFuture = - policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1); + policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1, directExecutor()); assertThrows(ExecutionException.class, statusFuture::get); } @@ -193,7 +194,7 @@ public void testPerServiceAsync_cancelledFuture_propagatesStatus() { .build(); ListenableFuture statusFuture = - policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1); + policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1, directExecutor()); assertThrows(CancellationException.class, statusFuture::get); } @@ -223,7 +224,7 @@ public void testPerServiceAsync_interrupted_cancelledFuture() { })) .build(); ListenableFuture statusFuture = - policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1); + policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1, directExecutor()); assertThrows(InterruptedException.class, statusFuture::get); listeningExecutorService.shutdownNow(); @@ -303,7 +304,7 @@ SERVICE2, asyncPolicy((uid) -> { anotherUid ? Status.OK : Status.PERMISSION_DENIED, - MoreExecutors.directExecutor()); + directExecutor()); })) .build(); @@ -335,7 +336,7 @@ private static Status.Code checkAuthorizationForServiceAsync( int callerUid, String service) throws ExecutionException { ListenableFuture statusFuture = - policy.checkAuthorizationForServiceAsync(callerUid, service); + policy.checkAuthorizationForServiceAsync(callerUid, service, directExecutor()); return Uninterruptibles.getUninterruptibly(statusFuture).getCode(); } From f5cdc31f81a650170df677cdc91642c39e0355c8 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Wed, 12 Jun 2024 09:59:15 -0700 Subject: [PATCH 02/21] docs whitespace --- .../src/main/java/io/grpc/binder/ServerSecurityPolicy.java | 5 +++-- .../io/grpc/binder/internal/BinderTransportSecurity.java | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java index 96e1dfe5d7c..7c92f8a22b7 100644 --- a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java +++ b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java @@ -67,12 +67,13 @@ public Status checkAuthorizationForService(int uid, String serviceName) { * * @param uid The Android UID to authenticate. * @param serviceName The name of the gRPC service being called. - * @param offloadExecutor used for evaluating the relevant SecurityPolicy if not natively async + * @param offloadExecutor for evaluating the relevant SecurityPolicy if not natively async * @return a future with the result of the authorization check. A failed future represents a * failure to perform the authorization check, not that the access is denied. */ @CheckReturnValue - ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName, Executor offloadExecutor) { + ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName, + Executor offloadExecutor) { SecurityPolicy securityPolicy = perServicePolicies.getOrDefault(serviceName, defaultPolicy); if (securityPolicy instanceof AsyncSecurityPolicy) { return ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(uid); diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java index 72a02c92ff6..86ec7415197 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -220,7 +220,7 @@ public void onFailure(Throwable t) { * *

This class provides the asynchronous version of {@link io.grpc.binder.SecurityPolicy}, * allowing implementations of authorization logic that involves slow or asynchronous calls - * without necessarily blocking the calling thread. + * without blocking the calling thread. * * @see io.grpc.binder.SecurityPolicy */ @@ -231,6 +231,8 @@ public interface ServerPolicyChecker { *

This method never throws an exception. If the execution of the security policy check * fails, a failed future with such exception is returned. * + *

This method never blocks the calling thread. + * * @param uid The Android UID to authenticate. * @param serviceName The name of the gRPC service being called. * @return a future with the result of the authorization check. A failed future represents a From c253a4be492f4157cda6be7ceb50c8af5a85e4ee Mon Sep 17 00:00:00 2001 From: John Cormie Date: Wed, 12 Jun 2024 10:08:27 -0700 Subject: [PATCH 03/21] unused imports --- binder/src/main/java/io/grpc/binder/BinderInternal.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderInternal.java b/binder/src/main/java/io/grpc/binder/BinderInternal.java index 6d610d172f1..fb78da4cd3b 100644 --- a/binder/src/main/java/io/grpc/binder/BinderInternal.java +++ b/binder/src/main/java/io/grpc/binder/BinderInternal.java @@ -17,11 +17,8 @@ package io.grpc.binder; import android.os.IBinder; -import com.google.common.util.concurrent.ListenableFuture; import io.grpc.Internal; -import io.grpc.Status; import io.grpc.binder.internal.BinderTransportSecurity; -import io.grpc.binder.internal.BinderTransportSecurity.ServerPolicyChecker; import java.util.concurrent.Executor; /** From 627d5d51801d2f4b4216151d290f470a2def0013 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Wed, 12 Jun 2024 10:11:17 -0700 Subject: [PATCH 04/21] format --- binder/src/main/java/io/grpc/binder/BinderInternal.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderInternal.java b/binder/src/main/java/io/grpc/binder/BinderInternal.java index fb78da4cd3b..157016dc91b 100644 --- a/binder/src/main/java/io/grpc/binder/BinderInternal.java +++ b/binder/src/main/java/io/grpc/binder/BinderInternal.java @@ -36,8 +36,8 @@ public static void setIBinder(IBinderReceiver receiver, IBinder binder) { /** * Creates a {@link BinderTransportSecurity.ServerPolicyChecker} from a - * {@link ServerSecurityPolicy}. This exposes to callers an interface to check security policies - * without causing hard dependencies on a specific class. + * {@link ServerSecurityPolicy} and a default {@link Executor}. This exposes to callers an + * interface to check security policies without causing hard dependencies on a specific class. */ public static BinderTransportSecurity.ServerPolicyChecker createPolicyChecker( ServerSecurityPolicy securityPolicy, Executor executor) { From 5f90a9331ba0f0f67401564372fc4209cd7326a2 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 14 Jun 2024 12:18:51 -0700 Subject: [PATCH 05/21] let the BinderServer provide the executor after start --- .../java/io/grpc/binder/BinderInternal.java | 5 ++- .../io/grpc/binder/BinderServerBuilder.java | 6 +-- .../io/grpc/binder/internal/BinderServer.java | 15 +------- .../internal/BinderTransportSecurity.java | 37 +++++++++---------- 4 files changed, 25 insertions(+), 38 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderInternal.java b/binder/src/main/java/io/grpc/binder/BinderInternal.java index 157016dc91b..c50471e375a 100644 --- a/binder/src/main/java/io/grpc/binder/BinderInternal.java +++ b/binder/src/main/java/io/grpc/binder/BinderInternal.java @@ -40,7 +40,8 @@ public static void setIBinder(IBinderReceiver receiver, IBinder binder) { * interface to check security policies without causing hard dependencies on a specific class. */ public static BinderTransportSecurity.ServerPolicyChecker createPolicyChecker( - ServerSecurityPolicy securityPolicy, Executor executor) { - return (uid, serviceName) -> securityPolicy.checkAuthorizationForServiceAsync(uid, serviceName, executor); + ServerSecurityPolicy securityPolicy) { + return (uid, serviceName, executor) -> + securityPolicy.checkAuthorizationForServiceAsync(uid, serviceName, executor); } } diff --git a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java index ef1f6428afb..3d3f3ca3578 100644 --- a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java @@ -35,6 +35,7 @@ import java.io.File; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; +import javax.annotation.Nullable; /** * Builder for a server that services requests from an Android Service. @@ -182,10 +183,7 @@ public Server build() { checkState(!isBuilt, "BinderServerBuilder can only be used to build one server instance."); isBuilt = true; // We install the security interceptor last, so it's closest to the transport. - ObjectPool executorPool = serverImplBuilder.getExecutorPool(); - Executor executor = executorPool.getObject(); - BinderTransportSecurity.installAuthInterceptor(this, executor); - internalBuilder.setTerminationListener(() -> executorPool.returnObject(executor)); + BinderTransportSecurity.installAuthInterceptor(this); return super.build(); } } diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java index c5e7815893d..a3b0340e622 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java @@ -110,7 +110,7 @@ public synchronized void start(ServerListener serverListener) throws IOException listener = new ActiveTransportTracker(serverListener, this::onTermination); executorService = executorServicePool.getObject(); offloadExecutor = offloadExecutorPool.getObject(); - serverPolicyChecker = BinderInternal.createPolicyChecker(serverSecurityPolicy, offloadExecutor); + serverPolicyChecker = BinderInternal.createPolicyChecker(serverSecurityPolicy); } @Override @@ -179,7 +179,7 @@ public synchronized boolean handleTransaction(int code, Parcel parcel) { .set(BinderTransport.REMOTE_UID, callingUid) .set(BinderTransport.SERVER_AUTHORITY, listenAddress.getAuthority()) .set(BinderTransport.INBOUND_PARCELABLE_POLICY, inboundParcelablePolicy); - BinderTransportSecurity.attachAuthAttrs(attrsBuilder, callingUid, serverPolicyChecker); + BinderTransportSecurity.attachAuthAttrs(attrsBuilder, callingUid, serverPolicyChecker, offloadExecutor); // Create a new transport and let our listener know about it. BinderTransport.BinderServerTransport transport = new BinderTransport.BinderServerTransport( @@ -295,16 +295,5 @@ public Builder setInboundParcelablePolicy(InboundParcelablePolicy inboundParcela this.inboundParcelablePolicy = checkNotNull(inboundParcelablePolicy, "inboundParcelablePolicy"); return this; } - - /** - * Installs a callback that will be invoked when this server is {@link #shutdown()} and all of - * its transports are terminated. - * - *

Optional. - */ - public Builder setTerminationListener(Runnable terminationListener) { - this.terminationListener = checkNotNull(terminationListener, "terminationListener"); - return this; - } } } diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java index 86ec7415197..65a2c156a13 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -57,11 +57,10 @@ private BinderTransportSecurity() {} * Install a security policy on an about-to-be created server. * * @param serverBuilder The ServerBuilder being used to create the server. - * @param executor The executor in which the authorization result will be handled. */ @Internal - public static void installAuthInterceptor(ServerBuilder serverBuilder, Executor executor) { - serverBuilder.intercept(new ServerAuthInterceptor(executor)); + public static void installAuthInterceptor(ServerBuilder serverBuilder) { + serverBuilder.intercept(new ServerAuthInterceptor()); } /** @@ -74,11 +73,12 @@ public static void installAuthInterceptor(ServerBuilder serverBuilder, Execut */ @Internal public static void attachAuthAttrs( - Attributes.Builder builder, int remoteUid, ServerPolicyChecker serverPolicyChecker) { + Attributes.Builder builder, int remoteUid, ServerPolicyChecker serverPolicyChecker, + Executor offloadExecutor) { builder .set( TRANSPORT_AUTHORIZATION_STATE, - new TransportAuthorizationState(remoteUid, serverPolicyChecker)) + new TransportAuthorizationState(remoteUid, serverPolicyChecker, offloadExecutor)) .set(GrpcAttributes.ATTR_SECURITY_LEVEL, SecurityLevel.PRIVACY_AND_INTEGRITY); } @@ -88,25 +88,20 @@ public static void attachAuthAttrs( */ private static final class ServerAuthInterceptor implements ServerInterceptor { - private final Executor executor; - - ServerAuthInterceptor(Executor executor) { - this.executor = executor; - } - @Override public ServerCall.Listener interceptCall( ServerCall call, Metadata headers, ServerCallHandler next) { + TransportAuthorizationState transpoAuthState = + call.getAttributes().get(TRANSPORT_AUTHORIZATION_STATE); ListenableFuture authStatusFuture = - call.getAttributes() - .get(TRANSPORT_AUTHORIZATION_STATE) - .checkAuthorization(call.getMethodDescriptor()); + transpoAuthState.checkAuthorization(call.getMethodDescriptor()); // Most SecurityPolicy will have synchronous implementations that provide an // immediately-resolved Future. In that case, short-circuit to avoid unnecessary allocations // and asynchronous code if the authorization result is already present. if (!authStatusFuture.isDone()) { - return newServerCallListenerForPendingAuthResult(authStatusFuture, call, headers, next); + return newServerCallListenerForPendingAuthResult(authStatusFuture, + transpoAuthState.offloadExecutor, call, headers, next); } Status authStatus; @@ -131,6 +126,7 @@ public ServerCall.Listener interceptCall( private ServerCall.Listener newServerCallListenerForPendingAuthResult( ListenableFuture authStatusFuture, + Executor offloadExecutor, ServerCall call, Metadata headers, ServerCallHandler next) { @@ -154,7 +150,7 @@ public void onFailure(Throwable t) { Status.INTERNAL.withCause(t).withDescription("Authorization future failed"), new Metadata()); } - }, executor); + }, offloadExecutor); return listener; } } @@ -167,10 +163,13 @@ private static final class TransportAuthorizationState { private final int uid; private final ServerPolicyChecker serverPolicyChecker; private final ConcurrentHashMap> serviceAuthorization; + private final Executor offloadExecutor; - TransportAuthorizationState(int uid, ServerPolicyChecker serverPolicyChecker) { + TransportAuthorizationState(int uid, ServerPolicyChecker serverPolicyChecker, + Executor offloadExecutor) { this.uid = uid; this.serverPolicyChecker = serverPolicyChecker; + this.offloadExecutor = offloadExecutor; serviceAuthorization = new ConcurrentHashMap<>(8); } @@ -198,7 +197,7 @@ ListenableFuture checkAuthorization(MethodDescriptor method) { // TODO(10669): evaluate if there should be at most a single pending authorization check per // (uid, serviceName) pair at any given time. ListenableFuture authorization = - serverPolicyChecker.checkAuthorizationForServiceAsync(uid, serviceName); + serverPolicyChecker.checkAuthorizationForServiceAsync(uid, serviceName, offloadExecutor); if (useCache) { serviceAuthorization.putIfAbsent(serviceName, authorization); Futures.addCallback(authorization, new FutureCallback() { @@ -238,6 +237,6 @@ public interface ServerPolicyChecker { * @return a future with the result of the authorization check. A failed future represents a * failure to perform the authorization check, not that the access is denied. */ - ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName); + ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName, Executor executor); } } From dd546c4a273c896d0965cdacf5fe65d15df0345e Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 14 Jun 2024 12:20:59 -0700 Subject: [PATCH 06/21] revert javadoc --- binder/src/main/java/io/grpc/binder/BinderInternal.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderInternal.java b/binder/src/main/java/io/grpc/binder/BinderInternal.java index c50471e375a..584ade387ac 100644 --- a/binder/src/main/java/io/grpc/binder/BinderInternal.java +++ b/binder/src/main/java/io/grpc/binder/BinderInternal.java @@ -36,8 +36,8 @@ public static void setIBinder(IBinderReceiver receiver, IBinder binder) { /** * Creates a {@link BinderTransportSecurity.ServerPolicyChecker} from a - * {@link ServerSecurityPolicy} and a default {@link Executor}. This exposes to callers an - * interface to check security policies without causing hard dependencies on a specific class. + * {@link ServerSecurityPolicy}. This exposes to callers an interface to check security policies + * without causing hard dependencies on a specific class. */ public static BinderTransportSecurity.ServerPolicyChecker createPolicyChecker( ServerSecurityPolicy securityPolicy) { From eb066bdce65672de39f337dc8a8375b1f87f88c3 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 14 Jun 2024 12:22:03 -0700 Subject: [PATCH 07/21] javadoc --- binder/src/main/java/io/grpc/binder/BinderServerBuilder.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java index 3d3f3ca3578..163e77f4332 100644 --- a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java @@ -153,8 +153,9 @@ public BinderServerBuilder useTransportSecurity(File certChain, File privateKey) /** * Provides a custom executor that will be used for operations that block or are expensive, to - * avoid blocking asynchronous code paths. For example, SecurityPolicy evaluation uses this - * executor (many implementations require one or more IPC round trips to Android's system server). + * avoid doing such things on asynchronous code paths. For example, SecurityPolicy evaluation + * uses this executor (many implementations require one or more IPC round trips to Android's + * system server). * *

It's an optional parameter. If the user has not provided an executor when the channel is * built, the builder will use a static cached thread pool. From ceb8c3e4e1d4e883ad0c8898a5572b3ed5e11b11 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 14 Jun 2024 12:26:55 -0700 Subject: [PATCH 08/21] more reverts --- .../java/io/grpc/binder/internal/BinderServer.java | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java index a3b0340e622..63b77713b4b 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java @@ -68,16 +68,12 @@ public final class BinderServer implements InternalServer, LeakSafeOneWayBinder. private final ImmutableList streamTracerFactories; private final AndroidComponentAddress listenAddress; private final LeakSafeOneWayBinder hostServiceBinder; - private final ServerSecurityPolicy serverSecurityPolicy; + private final BinderTransportSecurity.ServerPolicyChecker serverPolicyChecker; private final InboundParcelablePolicy inboundParcelablePolicy; - private final Runnable terminationListener; @GuardedBy("this") private ServerListener listener; - @GuardedBy("this") - private BinderTransportSecurity.ServerPolicyChecker serverPolicyChecker; - @GuardedBy("this") private ScheduledExecutorService executorService; @@ -94,9 +90,8 @@ private BinderServer(Builder builder) { builder.offloadExecutorPool : builder.executorServicePool; this.streamTracerFactories = ImmutableList.copyOf(checkNotNull(builder.streamTracerFactories, "streamTracerFactories")); - this.serverSecurityPolicy = builder.serverSecurityPolicy; + this.serverPolicyChecker = BinderInternal.createPolicyChecker(builder.serverSecurityPolicy); this.inboundParcelablePolicy = builder.inboundParcelablePolicy; - this.terminationListener = builder.terminationListener; hostServiceBinder = new LeakSafeOneWayBinder(this); } @@ -110,7 +105,6 @@ public synchronized void start(ServerListener serverListener) throws IOException listener = new ActiveTransportTracker(serverListener, this::onTermination); executorService = executorServicePool.getObject(); offloadExecutor = offloadExecutorPool.getObject(); - serverPolicyChecker = BinderInternal.createPolicyChecker(serverSecurityPolicy); } @Override @@ -148,7 +142,6 @@ public synchronized void shutdown() { private void onTermination() { offloadExecutor = offloadExecutorPool.returnObject(offloadExecutor); - terminationListener.run(); } @Override @@ -226,7 +219,6 @@ public static class Builder { SharedResourcePool.forResource(GrpcUtil.TIMER_SERVICE); ServerSecurityPolicy serverSecurityPolicy = SecurityPolicies.serverInternalOnly(); InboundParcelablePolicy inboundParcelablePolicy = InboundParcelablePolicy.DEFAULT; - Runnable terminationListener = () -> {}; public BinderServer build() { return new BinderServer(this); From b212d524fc40e8571c662e434dfc17c5085e3ee9 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 14 Jun 2024 12:37:16 -0700 Subject: [PATCH 09/21] cleanups --- binder/src/main/java/io/grpc/binder/BinderInternal.java | 1 - .../main/java/io/grpc/binder/BinderServerBuilder.java | 2 -- .../main/java/io/grpc/binder/ServerSecurityPolicy.java | 4 ++-- .../java/io/grpc/binder/internal/BinderTransport.java | 2 +- .../io/grpc/binder/internal/BinderTransportSecurity.java | 9 +++++++-- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderInternal.java b/binder/src/main/java/io/grpc/binder/BinderInternal.java index 584ade387ac..d6a8198380e 100644 --- a/binder/src/main/java/io/grpc/binder/BinderInternal.java +++ b/binder/src/main/java/io/grpc/binder/BinderInternal.java @@ -19,7 +19,6 @@ import android.os.IBinder; import io.grpc.Internal; import io.grpc.binder.internal.BinderTransportSecurity; -import java.util.concurrent.Executor; /** * Helper class to expose IBinderReceiver methods for legacy internal builders. diff --git a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java index 163e77f4332..d822028c411 100644 --- a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java @@ -30,12 +30,10 @@ import io.grpc.binder.internal.BinderTransportSecurity; import io.grpc.internal.FixedObjectPool; import io.grpc.internal.ServerImplBuilder; -import io.grpc.internal.ObjectPool; import java.io.File; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; -import javax.annotation.Nullable; /** * Builder for a server that services requests from an Android Service. diff --git a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java index 7c92f8a22b7..4ef221e2d31 100644 --- a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java +++ b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java @@ -65,9 +65,9 @@ public Status checkAuthorizationForService(int uid, String serviceName) { *

This method never throws an exception. If the execution of the security policy check * fails, a failed future with such exception is returned. * - * @param uid The Android UID to authenticate. + * @param uid The authentic Android UID to authorize. * @param serviceName The name of the gRPC service being called. - * @param offloadExecutor for evaluating the relevant SecurityPolicy if not natively async + * @param offloadExecutor for evaluating the relevant SecurityPolicy if it's not natively async * @return a future with the result of the authorization check. A failed future represents a * failure to perform the authorization check, not that the access is denied. */ diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java index debe6d539b7..9deb2bfaea1 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -653,7 +653,7 @@ private synchronized void onReadyTimeout() { } @Override - public ClientStream newStream( + public synchronized ClientStream newStream( final MethodDescriptor method, final Metadata headers, final CallOptions callOptions, diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java index 65a2c156a13..e83dca942c6 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -165,6 +165,9 @@ private static final class TransportAuthorizationState { private final ConcurrentHashMap> serviceAuthorization; private final Executor offloadExecutor; + /** + * @param offloadExecutor must remain valid for the lifetime of the associated transport + */ TransportAuthorizationState(int uid, ServerPolicyChecker serverPolicyChecker, Executor offloadExecutor) { this.uid = uid; @@ -232,11 +235,13 @@ public interface ServerPolicyChecker { * *

This method never blocks the calling thread. * - * @param uid The Android UID to authenticate. + * @param uid The authentic Android UID to authorize. * @param serviceName The name of the gRPC service being called. + * @param offloadExecutor used for blocking or expensive work if necessary * @return a future with the result of the authorization check. A failed future represents a * failure to perform the authorization check, not that the access is denied. */ - ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName, Executor executor); + ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName, + Executor offloadExecutor); } } From ef2b79e46044770aedd665a5a30d4e02453331c9 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 14 Jun 2024 12:45:43 -0700 Subject: [PATCH 10/21] clean --- .../src/main/java/io/grpc/binder/internal/BinderServer.java | 5 +++-- .../io/grpc/binder/internal/BinderTransportSecurity.java | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java index 63b77713b4b..fbf7af2ff91 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java @@ -78,7 +78,7 @@ public final class BinderServer implements InternalServer, LeakSafeOneWayBinder. private ScheduledExecutorService executorService; @GuardedBy("this") - private Executor offloadExecutor; + private Executor offloadExecutor; // != null between start() and onTermination(). @GuardedBy("this") private boolean shutdown; @@ -172,7 +172,8 @@ public synchronized boolean handleTransaction(int code, Parcel parcel) { .set(BinderTransport.REMOTE_UID, callingUid) .set(BinderTransport.SERVER_AUTHORITY, listenAddress.getAuthority()) .set(BinderTransport.INBOUND_PARCELABLE_POLICY, inboundParcelablePolicy); - BinderTransportSecurity.attachAuthAttrs(attrsBuilder, callingUid, serverPolicyChecker, offloadExecutor); + BinderTransportSecurity.attachAuthAttrs(attrsBuilder, callingUid, serverPolicyChecker, + offloadExecutor); // Create a new transport and let our listener know about it. BinderTransport.BinderServerTransport transport = new BinderTransport.BinderServerTransport( diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java index e83dca942c6..f2f820f405b 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -222,7 +222,7 @@ public void onFailure(Throwable t) { * *

This class provides the asynchronous version of {@link io.grpc.binder.SecurityPolicy}, * allowing implementations of authorization logic that involves slow or asynchronous calls - * without blocking the calling thread. + * without ever blocking the calling thread. * * @see io.grpc.binder.SecurityPolicy */ From a76790f81f8d7123b17bd3bd4db513d4a0ec7ad0 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Fri, 14 Jun 2024 14:39:12 -0700 Subject: [PATCH 11/21] real executor in tests revert some doc changes --- .../java/io/grpc/binder/BinderInternal.java | 2 +- .../io/grpc/binder/ServerSecurityPolicy.java | 2 +- .../internal/BinderTransportSecurity.java | 3 +- .../grpc/binder/ServerSecurityPolicyTest.java | 35 ++++++++++++------- 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderInternal.java b/binder/src/main/java/io/grpc/binder/BinderInternal.java index d6a8198380e..60518984650 100644 --- a/binder/src/main/java/io/grpc/binder/BinderInternal.java +++ b/binder/src/main/java/io/grpc/binder/BinderInternal.java @@ -39,7 +39,7 @@ public static void setIBinder(IBinderReceiver receiver, IBinder binder) { * without causing hard dependencies on a specific class. */ public static BinderTransportSecurity.ServerPolicyChecker createPolicyChecker( - ServerSecurityPolicy securityPolicy) { + ServerSecurityPolicy securityPolicy) { return (uid, serviceName, executor) -> securityPolicy.checkAuthorizationForServiceAsync(uid, serviceName, executor); } diff --git a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java index 4ef221e2d31..83cc819d03a 100644 --- a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java +++ b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java @@ -65,7 +65,7 @@ public Status checkAuthorizationForService(int uid, String serviceName) { *

This method never throws an exception. If the execution of the security policy check * fails, a failed future with such exception is returned. * - * @param uid The authentic Android UID to authorize. + * @param uid The Android UID to authenticate. * @param serviceName The name of the gRPC service being called. * @param offloadExecutor for evaluating the relevant SecurityPolicy if it's not natively async * @return a future with the result of the authorization check. A failed future represents a diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java index f2f820f405b..91cbbe48478 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -70,6 +70,7 @@ public static void installAuthInterceptor(ServerBuilder serverBuilder) { * @param builder The {@link Attributes.Builder} for the transport being created. * @param remoteUid The remote UID of the transport. * @param serverPolicyChecker The policy checker for this transport. + * @param offloadExecutor used for blocking or expensive work if necessary */ @Internal public static void attachAuthAttrs( @@ -235,7 +236,7 @@ public interface ServerPolicyChecker { * *

This method never blocks the calling thread. * - * @param uid The authentic Android UID to authorize. + * @param uid The Android UID to authenticate. * @param serviceName The name of the gRPC service being called. * @param offloadExecutor used for blocking or expensive work if necessary * @return a future with the result of the authorization check. A failed future represents a diff --git a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java index 56042fbdfdf..261379171b4 100644 --- a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java +++ b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java @@ -17,11 +17,16 @@ package io.grpc.binder; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.util.concurrent.MoreExecutors.directExecutor; import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; +import static org.robolectric.Shadows.shadowOf; + +import android.app.Application; +import android.os.Looper; import android.os.Process; +import androidx.core.content.ContextCompat; +import androidx.test.core.app.ApplicationProvider; import com.google.common.base.Function; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -30,7 +35,7 @@ import com.google.common.util.concurrent.Uninterruptibles; import io.grpc.Status; -import io.grpc.Status.Code; +import java.util.concurrent.Executor; import org.junit.Test; import org.junit.runner.RunWith; import org.robolectric.RobolectricTestRunner; @@ -52,6 +57,8 @@ public final class ServerSecurityPolicyTest { private static final int OTHER_UID = MY_UID + 1; ServerSecurityPolicy policy; + final Executor executor = ContextCompat.getMainExecutor( + ApplicationProvider.getApplicationContext()); @Test public void testDefaultInternalOnly() throws Exception { @@ -155,7 +162,7 @@ public void testPerServiceAsync() throws Exception { // of futures gets properly handled. ListenableFuture dependency = Futures.immediateVoidFuture(); return Futures - .transform(dependency, unused -> Status.OK, directExecutor()); + .transform(dependency, unused -> Status.OK, executor); })) .build(); @@ -181,8 +188,8 @@ public void testPerService_failedSecurityPolicyFuture_returnsAFailedFuture() { .build(); ListenableFuture statusFuture = - policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1, directExecutor()); - + policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1, executor); + shadowOf(Looper.getMainLooper()).idle(); assertThrows(ExecutionException.class, statusFuture::get); } @@ -194,8 +201,8 @@ public void testPerServiceAsync_cancelledFuture_propagatesStatus() { .build(); ListenableFuture statusFuture = - policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1, directExecutor()); - + policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1, executor); + shadowOf(Looper.getMainLooper()).idle(); assertThrows(CancellationException.class, statusFuture::get); } @@ -224,8 +231,8 @@ public void testPerServiceAsync_interrupted_cancelledFuture() { })) .build(); ListenableFuture statusFuture = - policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1, directExecutor()); - + policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1, executor); + shadowOf(Looper.getMainLooper()).idle(); assertThrows(InterruptedException.class, statusFuture::get); listeningExecutorService.shutdownNow(); } @@ -304,7 +311,7 @@ SERVICE2, asyncPolicy((uid) -> { anotherUid ? Status.OK : Status.PERMISSION_DENIED, - directExecutor()); + MoreExecutors.directExecutor()); })) .build(); @@ -331,13 +338,15 @@ SERVICE2, asyncPolicy((uid) -> { * Shortcut for invoking {@link ServerSecurityPolicy#checkAuthorizationForServiceAsync} without * dealing with concurrency details. Returns a {link @Status.Code} for convenience. */ - private static Status.Code checkAuthorizationForServiceAsync( + private Status.Code checkAuthorizationForServiceAsync( ServerSecurityPolicy policy, int callerUid, String service) throws ExecutionException { ListenableFuture statusFuture = - policy.checkAuthorizationForServiceAsync(callerUid, service, directExecutor()); - return Uninterruptibles.getUninterruptibly(statusFuture).getCode(); + policy.checkAuthorizationForServiceAsync(callerUid, service, executor); + shadowOf(Looper.getMainLooper()).idle(); + return Futures.getDone(statusFuture).getCode(); + // return Uninterruptibles.getUninterruptibly(statusFuture).getCode(); } private static SecurityPolicy policy(Function func) { From 7a3dd8a7d43d786679b837b113e58f74dc8ab985 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 18 Jun 2024 19:19:47 -0700 Subject: [PATCH 12/21] finish rebasing --- .../java/io/grpc/binder/BinderServerBuilder.java | 5 +++-- .../binder/internal/BinderTransportSecurity.java | 12 ++++++------ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java index 1946dff1290..45cd846378e 100644 --- a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java @@ -32,6 +32,7 @@ import io.grpc.internal.ServerImplBuilder; import java.io.File; +import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; /** @@ -157,8 +158,8 @@ public BinderServerBuilder useTransportSecurity(File certChain, File privateKey) *

It's an optional parameter. If the user has not provided an executor when the channel is * built, the builder will use a static cached thread pool. * - *

The channel won't take ownership of the given executor. Callers must ensure that it lives at - * least as long as the servers you build. + *

The channel won't take ownership of the given executor. Callers must ensure that it lives + * until the servers you build terminate. * * @return this */ diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java index 74f862c53ce..d0beffd3c06 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -76,7 +76,8 @@ public static void attachAuthAttrs( Attributes.Builder builder, int remoteUid, ServerPolicyChecker serverPolicyChecker, - Executor executor) { + Executor executor, + Executor offloadExecutor) { builder .set( TRANSPORT_AUTHORIZATION_STATE, @@ -98,12 +99,11 @@ public ServerCall.Listener interceptCall( ListenableFuture authStatusFuture = transportAuthState.checkAuthorization(call.getMethodDescriptor()); - // Most SecurityPolicy will have synchronous implementations that provide an - // immediately-resolved Future. In that case, short-circuit to avoid unnecessary allocations - // and asynchronous code if the authorization result is already present. + // Authorization decisions are cached and so this future will commonly already be complete. + // In that case, we have a fast path below that avoids unnecessary allocations and + // asynchronous code if the authorization result is already known. if (!authStatusFuture.isDone()) { - return newServerCallListenerForPendingAuthResult(authStatusFuture, - transpoAuthState.executor, call, headers, next); + return newServerCallListenerForPendingAuthResult(authStatusFuture, transportAuthState.executor, call, headers, next); } Status authStatus; From f958c8ec7564c1a5ccea6f3e3fde508d23f6d909 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 18 Jun 2024 19:24:39 -0700 Subject: [PATCH 13/21] google-java-format --- .../io/grpc/binder/BinderServerBuilder.java | 6 ++-- .../io/grpc/binder/ServerSecurityPolicy.java | 4 +-- .../io/grpc/binder/internal/BinderServer.java | 6 ++-- .../internal/BinderTransportSecurity.java | 19 ++++++---- .../grpc/binder/ServerSecurityPolicyTest.java | 36 +++++++++---------- 5 files changed, 37 insertions(+), 34 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java index 45cd846378e..22bea1f3291 100644 --- a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java @@ -151,9 +151,9 @@ public BinderServerBuilder useTransportSecurity(File certChain, File privateKey) /** * Provides a custom executor that will be used for operations that block or are expensive, to - * avoid doing such things on asynchronous code paths. For example, SecurityPolicy evaluation - * uses this executor (many implementations require one or more IPC round trips to Android's - * system server). + * avoid doing such things on asynchronous code paths. For example, {@link SecurityPolicy}s may be + * evaluated on this executor (many implementations require one or more blocking IPC round trips + * to Android's system server). * *

It's an optional parameter. If the user has not provided an executor when the channel is * built, the builder will use a static cached thread pool. diff --git a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java index 83cc819d03a..3d3b534bf0f 100644 --- a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java +++ b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java @@ -72,8 +72,8 @@ public Status checkAuthorizationForService(int uid, String serviceName) { * failure to perform the authorization check, not that the access is denied. */ @CheckReturnValue - ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName, - Executor offloadExecutor) { + ListenableFuture checkAuthorizationForServiceAsync( + int uid, String serviceName, Executor offloadExecutor) { SecurityPolicy securityPolicy = perServicePolicies.getOrDefault(serviceName, defaultPolicy); if (securityPolicy instanceof AsyncSecurityPolicy) { return ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(uid); diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java index 149ae034f62..d7f7a7b7312 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java @@ -93,8 +93,10 @@ private BinderServer(Builder builder) { this.listenAddress = checkNotNull(builder.listenAddress); this.executorPool = checkNotNull(builder.executorPool); this.executorServicePool = builder.executorServicePool; - this.offloadExecutorPool = builder.offloadExecutorPool != null ? - builder.offloadExecutorPool : builder.executorServicePool; + this.offloadExecutorPool = + builder.offloadExecutorPool != null + ? builder.offloadExecutorPool + : builder.executorServicePool; this.streamTracerFactories = ImmutableList.copyOf(checkNotNull(builder.streamTracerFactories, "streamTracerFactories")); this.serverPolicyChecker = BinderInternal.createPolicyChecker(builder.serverSecurityPolicy); diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java index d0beffd3c06..5ac248a5d1d 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -81,7 +81,8 @@ public static void attachAuthAttrs( builder .set( TRANSPORT_AUTHORIZATION_STATE, - new TransportAuthorizationState(remoteUid, serverPolicyChecker, executor, offloadExecutor)) + new TransportAuthorizationState( + remoteUid, serverPolicyChecker, executor, offloadExecutor)) .set(GrpcAttributes.ATTR_SECURITY_LEVEL, SecurityLevel.PRIVACY_AND_INTEGRITY); } @@ -103,7 +104,8 @@ public ServerCall.Listener interceptCall( // In that case, we have a fast path below that avoids unnecessary allocations and // asynchronous code if the authorization result is already known. if (!authStatusFuture.isDone()) { - return newServerCallListenerForPendingAuthResult(authStatusFuture, transportAuthState.executor, call, headers, next); + return newServerCallListenerForPendingAuthResult( + authStatusFuture, transportAuthState.executor, call, headers, next); } Status authStatus; @@ -173,8 +175,11 @@ private static final class TransportAuthorizationState { * @param executor used for calling into the application. Must outlive the transport. * @param offloadExecutor must remain valid for the lifetime of the associated transport */ - TransportAuthorizationState(int uid, ServerPolicyChecker serverPolicyChecker, - Executor executor, Executor offloadExecutor) { + TransportAuthorizationState( + int uid, + ServerPolicyChecker serverPolicyChecker, + Executor executor, + Executor offloadExecutor) { this.uid = uid; this.serverPolicyChecker = serverPolicyChecker; this.executor = executor; @@ -245,9 +250,9 @@ public interface ServerPolicyChecker { * @param serviceName The name of the gRPC service being called. * @param offloadExecutor used for blocking or expensive work if necessary * @return a future with the result of the authorization check. A failed future represents a - * failure to perform the authorization check, not that the access is denied. + * failure to perform the authorization check, not that the access is denied. */ - ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName, - Executor offloadExecutor); + ListenableFuture checkAuthorizationForServiceAsync( + int uid, String serviceName, Executor offloadExecutor); } } diff --git a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java index 261379171b4..3bebcf81261 100644 --- a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java +++ b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java @@ -18,11 +18,9 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; - import static org.junit.Assert.fail; import static org.robolectric.Shadows.shadowOf; -import android.app.Application; import android.os.Looper; import android.os.Process; import androidx.core.content.ContextCompat; @@ -32,19 +30,17 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; -import com.google.common.util.concurrent.Uninterruptibles; - import io.grpc.Status; -import java.util.concurrent.Executor; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.robolectric.RobolectricTestRunner; import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CancellationException; import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; import java.util.concurrent.Executors; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; @RunWith(RobolectricTestRunner.class) public final class ServerSecurityPolicyTest { @@ -57,8 +53,8 @@ public final class ServerSecurityPolicyTest { private static final int OTHER_UID = MY_UID + 1; ServerSecurityPolicy policy; - final Executor executor = ContextCompat.getMainExecutor( - ApplicationProvider.getApplicationContext()); + final Executor executor = + ContextCompat.getMainExecutor(ApplicationProvider.getApplicationContext()); @Test public void testDefaultInternalOnly() throws Exception { @@ -157,13 +153,15 @@ public void testPerService_legacyApi() { public void testPerServiceAsync() throws Exception { policy = ServerSecurityPolicy.newBuilder() - .servicePolicy(SERVICE2, asyncPolicy(uid -> { - // Add some extra future transformation to confirm that a chain - // of futures gets properly handled. - ListenableFuture dependency = Futures.immediateVoidFuture(); - return Futures - .transform(dependency, unused -> Status.OK, executor); - })) + .servicePolicy( + SERVICE2, + asyncPolicy( + uid -> { + // Add some extra future transformation to confirm that a chain + // of futures gets properly handled. + ListenableFuture dependency = Futures.immediateVoidFuture(); + return Futures.transform(dependency, unused -> Status.OK, executor); + })) .build(); assertThat(checkAuthorizationForServiceAsync(policy, MY_UID, SERVICE1)) @@ -339,9 +337,7 @@ SERVICE2, asyncPolicy((uid) -> { * dealing with concurrency details. Returns a {link @Status.Code} for convenience. */ private Status.Code checkAuthorizationForServiceAsync( - ServerSecurityPolicy policy, - int callerUid, - String service) throws ExecutionException { + ServerSecurityPolicy policy, int callerUid, String service) throws ExecutionException { ListenableFuture statusFuture = policy.checkAuthorizationForServiceAsync(callerUid, service, executor); shadowOf(Looper.getMainLooper()).idle(); From bea0cc46d24100773519d5670fa82479e2d09916 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 18 Jun 2024 19:32:37 -0700 Subject: [PATCH 14/21] rewording --- .../io/grpc/binder/internal/BinderTransportSecurity.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java index 5ac248a5d1d..09d4e66c88e 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -100,9 +100,9 @@ public ServerCall.Listener interceptCall( ListenableFuture authStatusFuture = transportAuthState.checkAuthorization(call.getMethodDescriptor()); - // Authorization decisions are cached and so this future will commonly already be complete. - // In that case, we have a fast path below that avoids unnecessary allocations and - // asynchronous code if the authorization result is already known. + // Auth decisions are cached so this future will often already be complete. In that case, we + // use a fast path below that avoids unnecessary allocations and asynchronous code if the + // authorization result is already known. if (!authStatusFuture.isDone()) { return newServerCallListenerForPendingAuthResult( authStatusFuture, transportAuthState.executor, call, headers, next); @@ -173,7 +173,7 @@ private static final class TransportAuthorizationState { /** * @param executor used for calling into the application. Must outlive the transport. - * @param offloadExecutor must remain valid for the lifetime of the associated transport + * @param offloadExecutor used to check a non-async SecurityPolicy. Must outlive the transport. */ TransportAuthorizationState( int uid, From f53c43ca989cc27ce0458e0ee54a9c57245f2a87 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 18 Jun 2024 19:35:17 -0700 Subject: [PATCH 15/21] members order --- .../src/main/java/io/grpc/binder/internal/BinderServer.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java index d7f7a7b7312..a6979c78a21 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java @@ -64,8 +64,8 @@ public final class BinderServer implements InternalServer, LeakSafeOneWayBinder. private static final Logger logger = Logger.getLogger(BinderServer.class.getName()); private final ObjectPool executorServicePool; - private final ObjectPool offloadExecutorPool; private final ObjectPool executorPool; + private final ObjectPool offloadExecutorPool; private final ImmutableList streamTracerFactories; private final AndroidComponentAddress listenAddress; @@ -83,8 +83,9 @@ public final class BinderServer implements InternalServer, LeakSafeOneWayBinder. @GuardedBy("this") private Executor executor; + @Nullable // Before start() and after termination. @GuardedBy("this") - private Executor offloadExecutor; // != null between start() and onTermination(). + private Executor offloadExecutor; @GuardedBy("this") private boolean shutdown; From cdff898d8f2551116c3c5f3d0086c21cc4576e65 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 18 Jun 2024 19:43:59 -0700 Subject: [PATCH 16/21] no need to change BinderInternal --- binder/src/main/java/io/grpc/binder/BinderInternal.java | 3 +-- binder/src/main/java/io/grpc/binder/internal/BinderServer.java | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderInternal.java b/binder/src/main/java/io/grpc/binder/BinderInternal.java index 60518984650..18af43ce2b3 100644 --- a/binder/src/main/java/io/grpc/binder/BinderInternal.java +++ b/binder/src/main/java/io/grpc/binder/BinderInternal.java @@ -40,7 +40,6 @@ public static void setIBinder(IBinderReceiver receiver, IBinder binder) { */ public static BinderTransportSecurity.ServerPolicyChecker createPolicyChecker( ServerSecurityPolicy securityPolicy) { - return (uid, serviceName, executor) -> - securityPolicy.checkAuthorizationForServiceAsync(uid, serviceName, executor); + return securityPolicy::checkAuthorizationForServiceAsync; } } diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java index a6979c78a21..d6e5d9df930 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java @@ -290,7 +290,7 @@ public Builder setExecutorServicePool( /** * Sets the executor to be used for blocking work. * - *

Optional. If unset, 'executorServicePool' will be used for this work (not recommended). + *

Optional. If unset, 'executorPool' will be used for this work (not recommended). */ public Builder setOffloadExecutorPool(ObjectPool offloadExecutorPool) { this.offloadExecutorPool = offloadExecutorPool; From b354bca5ec72c3b0e6708869fab736b2ef5643e4 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 18 Jun 2024 19:48:42 -0700 Subject: [PATCH 17/21] javadoc --- .../java/io/grpc/binder/internal/BinderTransportSecurity.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java index 09d4e66c88e..ac0f8ef0eaf 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -69,7 +69,7 @@ public static void installAuthInterceptor(ServerBuilder serverBuilder) { * @param remoteUid The remote UID of the transport. * @param serverPolicyChecker The policy checker for this transport. * @param executor used for calling into the application. Must outlive the transport. - * @param offloadExecutor used for blocking or expensive work if necessary + * @param offloadExecutor used for blocking or expensive work. Must outlive the transport. */ @Internal public static void attachAuthAttrs( From 25cd611eeccd6a96f6fa4f5ed0a2614a458d44e6 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Tue, 18 Jun 2024 19:49:41 -0700 Subject: [PATCH 18/21] comment --- .../java/io/grpc/binder/internal/BinderTransportSecurity.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java index ac0f8ef0eaf..8865181318b 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -101,7 +101,7 @@ public ServerCall.Listener interceptCall( transportAuthState.checkAuthorization(call.getMethodDescriptor()); // Auth decisions are cached so this future will often already be complete. In that case, we - // use a fast path below that avoids unnecessary allocations and asynchronous code if the + // use a fast path below that avoids unnecessary allocations and asynchronous code since the // authorization result is already known. if (!authStatusFuture.isDone()) { return newServerCallListenerForPendingAuthResult( From ae271064c5451a1b67f5bf5f651fa2bb69fd03d2 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Mon, 24 Jun 2024 11:40:00 -0700 Subject: [PATCH 19/21] gjf --- .../test/java/io/grpc/binder/ServerSecurityPolicyTest.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java index e6bc09b6b73..f5e904a1d7f 100644 --- a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java +++ b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java @@ -28,8 +28,6 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; - -import com.google.common.util.concurrent.Uninterruptibles; import io.grpc.Status; import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CancellationException; @@ -159,8 +157,7 @@ public void testPerServiceAsync() throws Exception { // Add some extra future transformation to confirm that a chain // of futures gets properly handled. ListenableFuture dependency = Futures.immediateVoidFuture(); - return Futures.transform( - dependency, unused -> Status.OK, executor); + return Futures.transform(dependency, unused -> Status.OK, executor); })) .build(); From 9a3e89ee6dd8347a6940eb947a06ecd047f08f84 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Mon, 24 Jun 2024 11:47:22 -0700 Subject: [PATCH 20/21] fix imports --- .../src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java index f5e904a1d7f..2272c8f312c 100644 --- a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java +++ b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java @@ -19,7 +19,9 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; +import static org.robolectric.Shadows.shadowOf; +import android.os.Looper; import android.os.Process; import androidx.core.content.ContextCompat; import androidx.test.core.app.ApplicationProvider; @@ -346,7 +348,6 @@ private Status.Code checkAuthorizationForServiceAsync( policy.checkAuthorizationForServiceAsync(callerUid, service, executor); shadowOf(Looper.getMainLooper()).idle(); return Futures.getDone(statusFuture).getCode(); - // return Uninterruptibles.getUninterruptibly(statusFuture).getCode(); } private static SecurityPolicy policy(Function func) { From 70709f6a1db8fcd2773a66a3fad7ec45daa5ba54 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Mon, 24 Jun 2024 12:33:47 -0700 Subject: [PATCH 21/21] javadoc --- .../io/grpc/binder/BinderServerBuilder.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java index bc5bb3eb02b..1be7eccc523 100644 --- a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java @@ -143,16 +143,17 @@ public BinderServerBuilder useTransportSecurity(File certChain, File privateKey) } /** - * Provides a custom executor that will be used for operations that block or are expensive, to - * avoid doing such things on asynchronous code paths. For example, {@link SecurityPolicy}s may be - * evaluated on this executor (many implementations require one or more blocking IPC round trips - * to Android's system server). + * Provides an executor to be used for operations that block or are expensive. * - *

It's an optional parameter. If the user has not provided an executor when the channel is - * built, the builder will use a static cached thread pool. + *

For example, {@link SecurityPolicy}s may be evaluated on this executor as implementations + * commonly require one or more blocking IPC round trips to Android's system server. This allows + * the host application to segregate its threads by workload. * - *

The channel won't take ownership of the given executor. Callers must ensure that it lives - * until the servers you build terminate. + *

Optional. By default, the executor associated with {@link ServerBuilder#executor(Executor)} + * will be used for this purpose. + * + *

The server won't take ownership of the given executor. Callers must ensure that it remains + * usable (not shutdown) until the built server terminates. * * @return this */