diff --git a/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java b/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java index 4a4657e781..2af7210e9a 100644 --- a/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java +++ b/binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java @@ -17,6 +17,7 @@ package io.grpc.binder.internal; import static com.google.common.truth.Truth.assertThat; +import static java.util.concurrent.TimeUnit.SECONDS; import android.content.Context; import android.os.DeadObjectException; @@ -24,8 +25,6 @@ import android.os.Parcel; import android.os.RemoteException; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; -import com.google.common.util.concurrent.Futures; -import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.concurrent.GuardedBy; @@ -39,13 +38,13 @@ import io.grpc.ServerServiceDefinition; import io.grpc.Status; import io.grpc.Status.Code; import io.grpc.binder.AndroidComponentAddress; -import io.grpc.binder.AsyncSecurityPolicy; import io.grpc.binder.BinderServerBuilder; import io.grpc.binder.HostServices; import io.grpc.binder.SecurityPolicy; import io.grpc.binder.internal.OneWayBinderProxies.BlackHoleOneWayBinderProxy; import io.grpc.binder.internal.OneWayBinderProxies.BlockingBinderDecorator; import io.grpc.binder.internal.OneWayBinderProxies.ThrowingOneWayBinderProxy; +import io.grpc.binder.internal.SettableAsyncSecurityPolicy.AuthRequest; import io.grpc.internal.ClientStream; import io.grpc.internal.ClientStreamListener; import io.grpc.internal.ClientTransportFactory.ClientTransportOptions; @@ -64,7 +63,6 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; import org.junit.After; import org.junit.Before; @@ -193,7 +191,7 @@ public final class BinderClientTransportTest { private static void shutdownAndTerminate(ExecutorService executorService) throws InterruptedException { executorService.shutdownNow(); - if (!executorService.awaitTermination(TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + if (!executorService.awaitTermination(TIMEOUT_SECONDS, SECONDS)) { throw new AssertionError("executor failed to terminate promptly"); } } @@ -375,17 +373,23 @@ public final class BinderClientTransportTest { @Test public void testBlackHoleSecurityPolicyConnectTimeout() throws Exception { + SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy(); transport = new BinderClientTransportBuilder() - .setSecurityPolicy(blockingSecurityPolicy) + .setSecurityPolicy(securityPolicy) .setReadyTimeoutMillis(1_234) .build(); transport.start(transportListener).run(); + // Take the next authRequest but don't respond to it, in order to trigger the ready timeout. + AuthRequest authRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS); + Status transportStatus = transportListener.awaitShutdown(); assertThat(transportStatus.getCode()).isEqualTo(Code.DEADLINE_EXCEEDED); assertThat(transportStatus.getDescription()).contains("1234"); transportListener.awaitTermination(); - blockingSecurityPolicy.provideNextCheckAuthorizationResult(Status.OK); + + // If the transport gave up waiting on auth, it should cancel its request. + assertThat(authRequest.isCancelled()).isTrue(); } @Test @@ -393,8 +397,8 @@ public final class BinderClientTransportTest { SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy(); transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build(); RuntimeException exception = new NullPointerException(); - securityPolicy.setAuthorizationException(exception); transport.start(transportListener).run(); + securityPolicy.takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS).setResult(exception); Status transportStatus = transportListener.awaitShutdown(); assertThat(transportStatus.getCode()).isEqualTo(Code.INTERNAL); assertThat(transportStatus.getCause()).isEqualTo(exception); @@ -405,13 +409,27 @@ public final class BinderClientTransportTest { public void testAsyncSecurityPolicySuccess() throws Exception { SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy(); transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build(); - securityPolicy.setAuthorizationResult(Status.PERMISSION_DENIED); transport.start(transportListener).run(); + securityPolicy + .takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS) + .setResult(Status.PERMISSION_DENIED); Status transportStatus = transportListener.awaitShutdown(); assertThat(transportStatus.getCode()).isEqualTo(Code.PERMISSION_DENIED); transportListener.awaitTermination(); } + @Test + public void testAsyncSecurityPolicyCancelledUponExternalTermination() throws Exception { + SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy(); + transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build(); + transport.start(transportListener).run(); + AuthRequest authRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS); + transport.shutdownNow(Status.UNAVAILABLE); // 'authRequest' remains unanswered! + transportListener.awaitShutdown(); + transportListener.awaitTermination(); + assertThat(authRequest.isCancelled()).isTrue(); + } + private static void startAndAwaitReady( BinderTransport.BinderClientTransport transport, TestTransportListener transportListener) throws Exception { @@ -433,7 +451,7 @@ public final class BinderClientTransportTest { } public Status awaitShutdown() throws Exception { - return shutdownStatus.get(TIMEOUT_SECONDS, TimeUnit.SECONDS); + return shutdownStatus.get(TIMEOUT_SECONDS, SECONDS); } @Override @@ -444,7 +462,7 @@ public final class BinderClientTransportTest { } public void awaitTermination() throws Exception { - isTerminated.get(TIMEOUT_SECONDS, TimeUnit.SECONDS); + isTerminated.get(TIMEOUT_SECONDS, SECONDS); } @Override @@ -455,7 +473,7 @@ public final class BinderClientTransportTest { } public void awaitReady() throws Exception { - isReady.get(TIMEOUT_SECONDS, TimeUnit.SECONDS); + isReady.get(TIMEOUT_SECONDS, SECONDS); } @Override @@ -571,25 +589,4 @@ public final class BinderClientTransportTest { } } } - - /** An AsyncSecurityPolicy that lets a test specify the outcome of checkAuthorizationAsync(). */ - static class SettableAsyncSecurityPolicy extends AsyncSecurityPolicy { - private SettableFuture result = SettableFuture.create(); - - public void clearAuthorizationResult() { - result = SettableFuture.create(); - } - - public boolean setAuthorizationResult(Status status) { - return result.set(status); - } - - public boolean setAuthorizationException(Throwable t) { - return result.setException(t); - } - - public ListenableFuture checkAuthorizationAsync(int uid) { - return Futures.nonCancellationPropagating(result); - } - } } 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 4b35137aa5..92a58b91cf 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -582,6 +582,8 @@ public abstract class BinderTransport implements IBinder.DeathRecipient { @GuardedBy("this") private ScheduledFuture readyTimeoutFuture; // != null iff timeout scheduled. + @GuardedBy("this") + @Nullable private ListenableFuture authResultFuture; // null before we check auth. /** * Constructs a new transport instance. @@ -756,6 +758,9 @@ public abstract class BinderTransport implements IBinder.DeathRecipient { readyTimeoutFuture.cancel(false); readyTimeoutFuture = null; } + if (authResultFuture != null) { + authResultFuture.cancel(false); // No effect if already complete. + } serviceBinding.unbind(); clientTransportListener.transportTerminated(); } @@ -775,13 +780,13 @@ public abstract class BinderTransport implements IBinder.DeathRecipient { shutdownInternal( Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true); } else { - ListenableFuture authFuture = + authResultFuture = (securityPolicy instanceof AsyncSecurityPolicy) ? ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(remoteUid) : Futures.submit( () -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor); Futures.addCallback( - authFuture, + authResultFuture, new FutureCallback() { @Override public void onSuccess(Status result) { diff --git a/binder/src/testFixtures/java/io/grpc/binder/internal/SettableAsyncSecurityPolicy.java b/binder/src/testFixtures/java/io/grpc/binder/internal/SettableAsyncSecurityPolicy.java new file mode 100644 index 0000000000..2cb22c2fdb --- /dev/null +++ b/binder/src/testFixtures/java/io/grpc/binder/internal/SettableAsyncSecurityPolicy.java @@ -0,0 +1,83 @@ +/* + * Copyright 2025 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.binder.internal; + +import static com.google.common.base.Preconditions.checkState; + +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; +import io.grpc.Status; +import io.grpc.binder.AsyncSecurityPolicy; +import java.util.concurrent.LinkedBlockingDeque; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +/** + * An {@link AsyncSecurityPolicy} that lets unit tests verify the exact order of authorization + * requests and respond to them one at a time. + */ +public class SettableAsyncSecurityPolicy extends AsyncSecurityPolicy { + private final LinkedBlockingDeque pendingRequests = new LinkedBlockingDeque<>(); + + @Override + public ListenableFuture checkAuthorizationAsync(int uid) { + AuthRequest request = new AuthRequest(uid); + pendingRequests.add(request); + return request.resultFuture; + } + + /** + * Waits for the next "check authorization" request to be made and returns it, throwing in case no + * request arrives in time. + */ + public AuthRequest takeNextAuthRequest(long timeout, TimeUnit unit) + throws InterruptedException, TimeoutException { + AuthRequest nextAuthRequest = pendingRequests.poll(timeout, unit); + if (nextAuthRequest == null) { + throw new TimeoutException(); + } + return nextAuthRequest; + } + + /** Represents a single call to {@link AsyncSecurityPolicy#checkAuthorizationAsync(int)}. */ + public static class AuthRequest { + + /** The argument passed to {@link AsyncSecurityPolicy#checkAuthorizationAsync(int)}. */ + public final int uid; + + private final SettableFuture resultFuture = SettableFuture.create(); + + private AuthRequest(int uid) { + this.uid = uid; + } + + /** Provides this SecurityPolicy's response to this authorization request. */ + public void setResult(Status result) { + checkState(resultFuture.set(result)); + } + + /** Simulates an exceptional response to this authorization request. */ + public void setResult(Throwable t) { + checkState(resultFuture.setException(t)); + } + + /** Tests if the future returned for this authorization request was cancelled by the caller. */ + public boolean isCancelled() { + return resultFuture.isCancelled(); + } + } +} \ No newline at end of file