From 8d758b689608d9fb53d4313b0bc43e58ebf3ee48 Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Thu, 18 Jan 2024 13:58:48 -0800 Subject: [PATCH 01/10] Force BinderSecurityTest to exercise async security policies codepaths. The usage of Futures.immediateFuture makes the implementation take several shortcuts to process server authorization. This leads to PendingAuthListener being under-tested. This commit adds delays to the futures in the hope that they will be triggered by the new codepaths. Before: https://fusion2.corp.google.com/invocations/fafaa334-2ac1-4d12-814c-33755fa08ad6/coverage After: https://fusion2.corp.google.com/invocations/6e3b4b41-461b-418d-8e9f-d9043dc3e5c6/coverage Part of #10566. --- .../io/grpc/binder/BinderSecurityTest.java | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java b/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java index 42796e2caed..2990396e6dd 100644 --- a/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java +++ b/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java @@ -47,7 +47,9 @@ import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; - +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; import org.junit.After; import org.junit.Before; @@ -66,6 +68,8 @@ public final class BinderSecurityTest { List> calls = new ArrayList<>(); CountingServerInterceptor countingServerInterceptor; + private final ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(); + @Before public void setupServiceDefinitionsAndMethods() { MethodDescriptor.Marshaller marshaller = @@ -102,6 +106,7 @@ public void tearDown() throws Exception { channel.shutdownNow(); } HostServices.awaitServiceShutdown(); + MoreExecutors.shutdownAndAwaitTermination(executor, 1, TimeUnit.MINUTES); } private void createChannel() throws Exception { @@ -276,8 +281,12 @@ public void testPerServicePolicy() throws Exception { public void testPerServicePolicyAsync() throws Exception { createChannel( ServerSecurityPolicy.newBuilder() - .servicePolicy("foo", asyncPolicy((uid) -> Futures.immediateFuture(true))) - .servicePolicy("bar", asyncPolicy((uid) -> Futures.immediateFuture(false))) + // Use delayed futures instead of immediate futures to force the implementation + // to execute the asynchronous code paths. The implementation has several + // shortcuts for immediately completed futures that could otherwise be triggered + // in these tests. + .servicePolicy("foo", asyncPolicy((uid) -> delayedFuture(true))) + .servicePolicy("bar", asyncPolicy((uid) -> delayedFuture(false))) .build(), SecurityPolicies.internalOnly()); @@ -335,6 +344,12 @@ public ListenableFuture checkAuthorizationAsync(int uid) { }; } + /** Returns a Future that is resolved with a value after some delay. */ + private ListenableFuture delayedFuture(T result) { + return Futures.scheduleAsync( + () -> Futures.immediateFuture(result), 1, TimeUnit.SECONDS, executor); + } + private final class CountingServerInterceptor implements ServerInterceptor { int numInterceptedCalls; From 6ff325f0c803acc14b8f4e82a372f299adb564b5 Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Fri, 19 Jan 2024 13:59:35 -0800 Subject: [PATCH 02/10] Use robolectric tests instead. --- binder/build.gradle | 1 + .../io/grpc/binder/BinderSecurityTest.java | 21 +- .../binder/BinderTransportSecurityTest.java | 235 ++++++++++++++++++ 3 files changed, 239 insertions(+), 18 deletions(-) create mode 100644 binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java diff --git a/binder/build.gradle b/binder/build.gradle index 5b593bfe5f5..62613b00cb5 100644 --- a/binder/build.gradle +++ b/binder/build.gradle @@ -51,6 +51,7 @@ dependencies { testImplementation libraries.robolectric testImplementation libraries.guava.testlib testImplementation libraries.truth + testImplementation project(':grpc-protobuf-lite') testImplementation project(':grpc-testing') testImplementation project(':grpc-inprocess') testImplementation testFixtures(project(':grpc-core')) diff --git a/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java b/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java index 2990396e6dd..42796e2caed 100644 --- a/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java +++ b/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java @@ -47,9 +47,7 @@ import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; + import javax.annotation.Nullable; import org.junit.After; import org.junit.Before; @@ -68,8 +66,6 @@ public final class BinderSecurityTest { List> calls = new ArrayList<>(); CountingServerInterceptor countingServerInterceptor; - private final ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(); - @Before public void setupServiceDefinitionsAndMethods() { MethodDescriptor.Marshaller marshaller = @@ -106,7 +102,6 @@ public void tearDown() throws Exception { channel.shutdownNow(); } HostServices.awaitServiceShutdown(); - MoreExecutors.shutdownAndAwaitTermination(executor, 1, TimeUnit.MINUTES); } private void createChannel() throws Exception { @@ -281,12 +276,8 @@ public void testPerServicePolicy() throws Exception { public void testPerServicePolicyAsync() throws Exception { createChannel( ServerSecurityPolicy.newBuilder() - // Use delayed futures instead of immediate futures to force the implementation - // to execute the asynchronous code paths. The implementation has several - // shortcuts for immediately completed futures that could otherwise be triggered - // in these tests. - .servicePolicy("foo", asyncPolicy((uid) -> delayedFuture(true))) - .servicePolicy("bar", asyncPolicy((uid) -> delayedFuture(false))) + .servicePolicy("foo", asyncPolicy((uid) -> Futures.immediateFuture(true))) + .servicePolicy("bar", asyncPolicy((uid) -> Futures.immediateFuture(false))) .build(), SecurityPolicies.internalOnly()); @@ -344,12 +335,6 @@ public ListenableFuture checkAuthorizationAsync(int uid) { }; } - /** Returns a Future that is resolved with a value after some delay. */ - private ListenableFuture delayedFuture(T result) { - return Futures.scheduleAsync( - () -> Futures.immediateFuture(result), 1, TimeUnit.SECONDS, executor); - } - private final class CountingServerInterceptor implements ServerInterceptor { int numInterceptedCalls; diff --git a/binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java b/binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java new file mode 100644 index 00000000000..c8c5ac62300 --- /dev/null +++ b/binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java @@ -0,0 +1,235 @@ +/* + * Copyright 2020 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; + +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.util.concurrent.MoreExecutors.directExecutor; + +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; +import com.google.common.util.concurrent.Uninterruptibles; +import com.google.protobuf.Empty; + +import android.app.Application; +import android.content.ComponentName; +import android.content.Intent; +import android.os.Handler; +import android.os.HandlerThread; +import android.os.IBinder; +import android.os.Looper; + +import io.grpc.CallOptions; +import io.grpc.ClientCall; +import io.grpc.ManagedChannel; +import io.grpc.Status; +import io.grpc.StatusRuntimeException; +import io.grpc.protobuf.lite.ProtoLiteUtils; + +import androidx.lifecycle.LifecycleService; +import androidx.test.core.app.ApplicationProvider; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.Robolectric; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.android.controller.ServiceController; + +import java.io.IOException; +import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.Executor; + +import io.grpc.MethodDescriptor; +import io.grpc.Server; +import io.grpc.ServerCallHandler; +import io.grpc.ServerMethodDefinition; +import io.grpc.ServerServiceDefinition; +import io.grpc.stub.ClientCalls; +import io.grpc.stub.ServerCalls; + +import static org.robolectric.Shadows.shadowOf; + +@RunWith(RobolectricTestRunner.class) +public final class BinderTransportSecurityTest { + + private static final String SERVICE_NAME = "fake_service"; + private static final String FULL_METHOD_NAME = "fake_service/fake_method"; + private final Application context = ApplicationProvider.getApplicationContext(); + private ServiceController controller; + private SomeService service; + private ManagedChannel channel; + + @Before + public void setUp() { + controller = Robolectric.buildService(SomeService.class); + service = controller.create().get(); + + AndroidComponentAddress listenAddress = AndroidComponentAddress.forContext(service); + channel = BinderChannelBuilder.forAddress(listenAddress, context).build(); + idleLoopers(); + } + + @After + public void tearDown() { + channel.shutdownNow(); + controller.destroy(); + } + + @Test + public void testAsyncServerSecurityPolicy_failed_returnsFailureStatus() throws Exception { + ListenableFuture status = makeCall(); + service.setSecurityPolicyStatusWhenReady(Status.ALREADY_EXISTS); + idleLoopers(); + + assertThat(status.get().getCode()).isEqualTo(Status.Code.ALREADY_EXISTS); + } + + @Test + public void testAsyncServerSecurityPolicy_allowed_returnsOkStatus() throws Exception { + ListenableFuture status = makeCall(); + service.setSecurityPolicyStatusWhenReady(Status.OK); + idleLoopers(); + + assertThat(status.get().getCode()).isEqualTo(Status.Code.OK); + } + + private void idleLoopers() { + service.idleLooper(); + shadowOf(Looper.getMainLooper()).idle(); + } + + private ListenableFuture makeCall() { + ClientCall call = + channel.newCall( + getMethodDescriptor(), + CallOptions.DEFAULT.withExecutor(service.getExecutor())); + ListenableFuture responseFuture = + ClientCalls.futureUnaryCall(call, Empty.getDefaultInstance()); + + idleLoopers(); + + return Futures.catching( + Futures.transform(responseFuture, unused -> Status.OK, directExecutor()), + StatusRuntimeException.class, + StatusRuntimeException::getStatus, + directExecutor()); + } + + private static MethodDescriptor getMethodDescriptor() { + MethodDescriptor.Marshaller marshaller = + ProtoLiteUtils.marshaller(Empty.getDefaultInstance()); + + return MethodDescriptor.newBuilder(marshaller, marshaller) + .setFullMethodName(FULL_METHOD_NAME) + .setType(MethodDescriptor.MethodType.UNARY) + .setSampledToLocalTracing(true) + .build(); + } + + private static class SomeService extends LifecycleService { + + private final IBinderReceiver binderReceiver = new IBinderReceiver(); + private final ArrayBlockingQueue> statuses = + new ArrayBlockingQueue<>(128); + private Server server; + private HandlerThread handlerThread; + private Handler handler; + + @Override + public void onCreate() { + super.onCreate(); + handlerThread = new HandlerThread("test_handler_thread"); + handlerThread.start(); + handler = new Handler(handlerThread.getLooper()); + + MethodDescriptor methodDesc = getMethodDescriptor(); + ServerCallHandler callHandler = + ServerCalls.asyncUnaryCall( + (req, respObserver) -> { + respObserver.onNext(req); + respObserver.onCompleted(); + }); + ServerMethodDefinition methodDef = + ServerMethodDefinition.create(methodDesc, callHandler); + ServerServiceDefinition def = ServerServiceDefinition.builder(SERVICE_NAME) + .addMethod(methodDef) + .build(); + + server = BinderServerBuilder.forAddress( + AndroidComponentAddress.forContext(this), + binderReceiver) + .addService(def) + .securityPolicy(ServerSecurityPolicy.newBuilder() + .servicePolicy(SERVICE_NAME, new AsyncSecurityPolicy() { + @Override + ListenableFuture checkAuthorizationAsync(int uid) { + return Futures.submitAsync(() -> { + SettableFuture status = SettableFuture.create(); + statuses.add(status); + return status; + }, getExecutor()); + } + }) + .build()) + .executor(getExecutor()) + .build(); + try { + server.start(); + } catch (IOException e) { + throw new IllegalStateException(e); + } + + Application context = ApplicationProvider.getApplicationContext(); + ComponentName componentName = new ComponentName(context, SomeService.class); + shadowOf(context) + .setComponentNameAndServiceForBindService( + componentName, checkNotNull(binderReceiver.get())); + } + + /** + * Returns an {@link Executor} under which all of the gRPC computations run under. The execution + * of any pending tasks on this executor can be triggered via {@link #idleLooper()}. + */ + Executor getExecutor() { + return handler::post; + } + + void idleLooper() { + shadowOf(handlerThread.getLooper()).idle(); + } + + void setSecurityPolicyStatusWhenReady(Status status) { + Uninterruptibles.takeUninterruptibly(statuses).set(status); + } + + @Override + public IBinder onBind(Intent intent) { + super.onBind(intent); + return checkNotNull(binderReceiver.get()); + } + + @Override + public void onDestroy() { + super.onDestroy(); + server.shutdownNow(); + handlerThread.quit(); + } + } +} From 38ffa18d9d1773c5e5b070c9997c7e5111f18dea Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Fri, 19 Jan 2024 14:29:52 -0800 Subject: [PATCH 03/10] fix typos --- .../io/grpc/binder/BinderTransportSecurityTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java b/binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java index c8c5ac62300..26ded767f9a 100644 --- a/binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java +++ b/binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java @@ -146,7 +146,7 @@ private static MethodDescriptor getMethodDescriptor() { private static class SomeService extends LifecycleService { private final IBinderReceiver binderReceiver = new IBinderReceiver(); - private final ArrayBlockingQueue> statuses = + private final ArrayBlockingQueue> statusesToSet = new ArrayBlockingQueue<>(128); private Server server; private HandlerThread handlerThread; @@ -182,7 +182,7 @@ public void onCreate() { ListenableFuture checkAuthorizationAsync(int uid) { return Futures.submitAsync(() -> { SettableFuture status = SettableFuture.create(); - statuses.add(status); + statusesToSet.add(status); return status; }, getExecutor()); } @@ -204,8 +204,8 @@ ListenableFuture checkAuthorizationAsync(int uid) { } /** - * Returns an {@link Executor} under which all of the gRPC computations run under. The execution - * of any pending tasks on this executor can be triggered via {@link #idleLooper()}. + * Returns an {@link Executor} under which all of the gRPC computations run. The execution of + * any pending tasks on this executor can be triggered via {@link #idleLooper()}. */ Executor getExecutor() { return handler::post; @@ -216,7 +216,7 @@ void idleLooper() { } void setSecurityPolicyStatusWhenReady(Status status) { - Uninterruptibles.takeUninterruptibly(statuses).set(status); + Uninterruptibles.takeUninterruptibly(statusesToSet).set(status); } @Override From e78a6ac973382380273bc200419c61733748e287 Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Mon, 22 Jan 2024 08:45:29 -0800 Subject: [PATCH 04/10] Rename test. Set other executors. --- .../binder/BinderTransportSecurityTest.java | 235 --------- .../binder/RobolectricBinderSecurityTest.java | 482 ++++++++++++++++++ 2 files changed, 482 insertions(+), 235 deletions(-) delete mode 100644 binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java create mode 100644 binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java diff --git a/binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java b/binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java deleted file mode 100644 index 26ded767f9a..00000000000 --- a/binder/src/test/java/io/grpc/binder/BinderTransportSecurityTest.java +++ /dev/null @@ -1,235 +0,0 @@ -/* - * Copyright 2020 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; - -import static com.google.common.base.Preconditions.checkNotNull; -import static com.google.common.truth.Truth.assertThat; -import static com.google.common.util.concurrent.MoreExecutors.directExecutor; - -import com.google.common.util.concurrent.Futures; -import com.google.common.util.concurrent.ListenableFuture; -import com.google.common.util.concurrent.SettableFuture; -import com.google.common.util.concurrent.Uninterruptibles; -import com.google.protobuf.Empty; - -import android.app.Application; -import android.content.ComponentName; -import android.content.Intent; -import android.os.Handler; -import android.os.HandlerThread; -import android.os.IBinder; -import android.os.Looper; - -import io.grpc.CallOptions; -import io.grpc.ClientCall; -import io.grpc.ManagedChannel; -import io.grpc.Status; -import io.grpc.StatusRuntimeException; -import io.grpc.protobuf.lite.ProtoLiteUtils; - -import androidx.lifecycle.LifecycleService; -import androidx.test.core.app.ApplicationProvider; - -import org.junit.After; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.robolectric.Robolectric; -import org.robolectric.RobolectricTestRunner; -import org.robolectric.android.controller.ServiceController; - -import java.io.IOException; -import java.util.concurrent.ArrayBlockingQueue; -import java.util.concurrent.Executor; - -import io.grpc.MethodDescriptor; -import io.grpc.Server; -import io.grpc.ServerCallHandler; -import io.grpc.ServerMethodDefinition; -import io.grpc.ServerServiceDefinition; -import io.grpc.stub.ClientCalls; -import io.grpc.stub.ServerCalls; - -import static org.robolectric.Shadows.shadowOf; - -@RunWith(RobolectricTestRunner.class) -public final class BinderTransportSecurityTest { - - private static final String SERVICE_NAME = "fake_service"; - private static final String FULL_METHOD_NAME = "fake_service/fake_method"; - private final Application context = ApplicationProvider.getApplicationContext(); - private ServiceController controller; - private SomeService service; - private ManagedChannel channel; - - @Before - public void setUp() { - controller = Robolectric.buildService(SomeService.class); - service = controller.create().get(); - - AndroidComponentAddress listenAddress = AndroidComponentAddress.forContext(service); - channel = BinderChannelBuilder.forAddress(listenAddress, context).build(); - idleLoopers(); - } - - @After - public void tearDown() { - channel.shutdownNow(); - controller.destroy(); - } - - @Test - public void testAsyncServerSecurityPolicy_failed_returnsFailureStatus() throws Exception { - ListenableFuture status = makeCall(); - service.setSecurityPolicyStatusWhenReady(Status.ALREADY_EXISTS); - idleLoopers(); - - assertThat(status.get().getCode()).isEqualTo(Status.Code.ALREADY_EXISTS); - } - - @Test - public void testAsyncServerSecurityPolicy_allowed_returnsOkStatus() throws Exception { - ListenableFuture status = makeCall(); - service.setSecurityPolicyStatusWhenReady(Status.OK); - idleLoopers(); - - assertThat(status.get().getCode()).isEqualTo(Status.Code.OK); - } - - private void idleLoopers() { - service.idleLooper(); - shadowOf(Looper.getMainLooper()).idle(); - } - - private ListenableFuture makeCall() { - ClientCall call = - channel.newCall( - getMethodDescriptor(), - CallOptions.DEFAULT.withExecutor(service.getExecutor())); - ListenableFuture responseFuture = - ClientCalls.futureUnaryCall(call, Empty.getDefaultInstance()); - - idleLoopers(); - - return Futures.catching( - Futures.transform(responseFuture, unused -> Status.OK, directExecutor()), - StatusRuntimeException.class, - StatusRuntimeException::getStatus, - directExecutor()); - } - - private static MethodDescriptor getMethodDescriptor() { - MethodDescriptor.Marshaller marshaller = - ProtoLiteUtils.marshaller(Empty.getDefaultInstance()); - - return MethodDescriptor.newBuilder(marshaller, marshaller) - .setFullMethodName(FULL_METHOD_NAME) - .setType(MethodDescriptor.MethodType.UNARY) - .setSampledToLocalTracing(true) - .build(); - } - - private static class SomeService extends LifecycleService { - - private final IBinderReceiver binderReceiver = new IBinderReceiver(); - private final ArrayBlockingQueue> statusesToSet = - new ArrayBlockingQueue<>(128); - private Server server; - private HandlerThread handlerThread; - private Handler handler; - - @Override - public void onCreate() { - super.onCreate(); - handlerThread = new HandlerThread("test_handler_thread"); - handlerThread.start(); - handler = new Handler(handlerThread.getLooper()); - - MethodDescriptor methodDesc = getMethodDescriptor(); - ServerCallHandler callHandler = - ServerCalls.asyncUnaryCall( - (req, respObserver) -> { - respObserver.onNext(req); - respObserver.onCompleted(); - }); - ServerMethodDefinition methodDef = - ServerMethodDefinition.create(methodDesc, callHandler); - ServerServiceDefinition def = ServerServiceDefinition.builder(SERVICE_NAME) - .addMethod(methodDef) - .build(); - - server = BinderServerBuilder.forAddress( - AndroidComponentAddress.forContext(this), - binderReceiver) - .addService(def) - .securityPolicy(ServerSecurityPolicy.newBuilder() - .servicePolicy(SERVICE_NAME, new AsyncSecurityPolicy() { - @Override - ListenableFuture checkAuthorizationAsync(int uid) { - return Futures.submitAsync(() -> { - SettableFuture status = SettableFuture.create(); - statusesToSet.add(status); - return status; - }, getExecutor()); - } - }) - .build()) - .executor(getExecutor()) - .build(); - try { - server.start(); - } catch (IOException e) { - throw new IllegalStateException(e); - } - - Application context = ApplicationProvider.getApplicationContext(); - ComponentName componentName = new ComponentName(context, SomeService.class); - shadowOf(context) - .setComponentNameAndServiceForBindService( - componentName, checkNotNull(binderReceiver.get())); - } - - /** - * Returns an {@link Executor} under which all of the gRPC computations run. The execution of - * any pending tasks on this executor can be triggered via {@link #idleLooper()}. - */ - Executor getExecutor() { - return handler::post; - } - - void idleLooper() { - shadowOf(handlerThread.getLooper()).idle(); - } - - void setSecurityPolicyStatusWhenReady(Status status) { - Uninterruptibles.takeUninterruptibly(statusesToSet).set(status); - } - - @Override - public IBinder onBind(Intent intent) { - super.onBind(intent); - return checkNotNull(binderReceiver.get()); - } - - @Override - public void onDestroy() { - super.onDestroy(); - server.shutdownNow(); - handlerThread.quit(); - } - } -} diff --git a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java new file mode 100644 index 00000000000..cb0887038ea --- /dev/null +++ b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java @@ -0,0 +1,482 @@ +/* + * Copyright 2020 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; + +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.util.concurrent.MoreExecutors.directExecutor; + +import com.google.common.collect.ImmutableList; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; +import com.google.common.util.concurrent.Uninterruptibles; +import com.google.protobuf.Empty; + +import android.app.Application; +import android.content.ComponentName; +import android.content.Intent; +import android.os.Handler; +import android.os.HandlerThread; +import android.os.IBinder; +import android.os.Looper; + +import io.grpc.CallOptions; +import io.grpc.ClientCall; +import io.grpc.ManagedChannel; +import io.grpc.Status; +import io.grpc.StatusRuntimeException; +import io.grpc.protobuf.lite.ProtoLiteUtils; + +import androidx.lifecycle.LifecycleService; +import androidx.test.core.app.ApplicationProvider; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.Robolectric; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.android.controller.ServiceController; + +import java.io.IOException; +import java.util.Collection; +import java.util.Comparator; +import java.util.List; +import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.Callable; +import java.util.concurrent.Delayed; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.time.Duration; +import java.util.stream.Collectors; + +import io.grpc.MethodDescriptor; +import io.grpc.Server; +import io.grpc.ServerCallHandler; +import io.grpc.ServerMethodDefinition; +import io.grpc.ServerServiceDefinition; +import io.grpc.stub.ClientCalls; +import io.grpc.stub.ServerCalls; + +import static org.robolectric.Shadows.shadowOf; + +@RunWith(RobolectricTestRunner.class) +public final class RobolectricBinderSecurityTest { + + private static final String SERVICE_NAME = "fake_service"; + private static final String FULL_METHOD_NAME = "fake_service/fake_method"; + private final Application context = ApplicationProvider.getApplicationContext(); + private ServiceController controller; + private SomeService service; + private ManagedChannel channel; + + @Before + public void setUp() { + controller = Robolectric.buildService(SomeService.class); + service = controller.create().get(); + + AndroidComponentAddress listenAddress = AndroidComponentAddress.forContext(service); + ScheduledExecutorService executor = service.getExecutor(); + channel = BinderChannelBuilder.forAddress(listenAddress, context) + .executor(executor) + .scheduledExecutorService(executor) + .offloadExecutor(executor) + .build(); + idleLoopers(); + } + + @After + public void tearDown() { + channel.shutdownNow(); + controller.destroy(); + } + + @Test + public void testAsyncServerSecurityPolicy_failed_returnsFailureStatus() throws Exception { + ListenableFuture status = makeCall(); + service.setSecurityPolicyStatusWhenReady(Status.ALREADY_EXISTS); + idleLoopers(); + + assertThat(Futures.getDone(status).getCode()).isEqualTo(Status.Code.ALREADY_EXISTS); + } + + @Test + public void testAsyncServerSecurityPolicy_allowed_returnsOkStatus() throws Exception { + ListenableFuture status = makeCall(); + service.setSecurityPolicyStatusWhenReady(Status.OK); + idleLoopers(); + + assertThat(Futures.getDone(status).getCode()).isEqualTo(Status.Code.OK); + } + + private void idleLoopers() { + service.idleLooper(); + shadowOf(Looper.getMainLooper()).idle(); + } + + private ListenableFuture makeCall() { + ClientCall call = + channel.newCall( + getMethodDescriptor(), + CallOptions.DEFAULT.withExecutor(service.getExecutor())); + ListenableFuture responseFuture = + ClientCalls.futureUnaryCall(call, Empty.getDefaultInstance()); + + idleLoopers(); + + return Futures.catching( + Futures.transform(responseFuture, unused -> Status.OK, directExecutor()), + StatusRuntimeException.class, + StatusRuntimeException::getStatus, + directExecutor()); + } + + private static MethodDescriptor getMethodDescriptor() { + MethodDescriptor.Marshaller marshaller = + ProtoLiteUtils.marshaller(Empty.getDefaultInstance()); + + return MethodDescriptor.newBuilder(marshaller, marshaller) + .setFullMethodName(FULL_METHOD_NAME) + .setType(MethodDescriptor.MethodType.UNARY) + .setSampledToLocalTracing(true) + .build(); + } + + private static class SomeService extends LifecycleService { + + private final IBinderReceiver binderReceiver = new IBinderReceiver(); + private final ArrayBlockingQueue> statusesToSet = + new ArrayBlockingQueue<>(128); + private Server server; + private HandlerThread handlerThread; + private Handler handler; + private final ScheduledExecutorService scheduledExecutorService = + new HandlerScheduledExecutorService(); + + @Override + public void onCreate() { + super.onCreate(); + handlerThread = new HandlerThread("test_handler_thread"); + handlerThread.start(); + handler = new Handler(handlerThread.getLooper()); + + MethodDescriptor methodDesc = getMethodDescriptor(); + ServerCallHandler callHandler = + ServerCalls.asyncUnaryCall( + (req, respObserver) -> { + respObserver.onNext(req); + respObserver.onCompleted(); + }); + ServerMethodDefinition methodDef = + ServerMethodDefinition.create(methodDesc, callHandler); + ServerServiceDefinition def = ServerServiceDefinition.builder(SERVICE_NAME) + .addMethod(methodDef) + .build(); + + server = BinderServerBuilder.forAddress( + AndroidComponentAddress.forContext(this), + binderReceiver) + .addService(def) + .securityPolicy(ServerSecurityPolicy.newBuilder() + .servicePolicy(SERVICE_NAME, new AsyncSecurityPolicy() { + @Override + ListenableFuture checkAuthorizationAsync(int uid) { + return Futures.submitAsync(() -> { + SettableFuture status = SettableFuture.create(); + statusesToSet.add(status); + return status; + }, getExecutor()); + } + }) + .build()) + .executor(getExecutor()) + .scheduledExecutorService(getExecutor()) + .build(); + try { + server.start(); + } catch (IOException e) { + throw new IllegalStateException(e); + } + + Application context = ApplicationProvider.getApplicationContext(); + ComponentName componentName = new ComponentName(context, SomeService.class); + shadowOf(context) + .setComponentNameAndServiceForBindService( + componentName, checkNotNull(binderReceiver.get())); + } + + /** + * Returns an {@link ScheduledExecutorService} under which all of the gRPC computations run. The + * execution of any pending tasks on this executor can be triggered via {@link #idleLooper()}. + */ + ScheduledExecutorService getExecutor() { + return scheduledExecutorService; + } + + void idleLooper() { + shadowOf(handlerThread.getLooper()).idle(); + } + + void setSecurityPolicyStatusWhenReady(Status status) { + Uninterruptibles.takeUninterruptibly(statusesToSet).set(status); + } + + @Override + public IBinder onBind(Intent intent) { + super.onBind(intent); + return checkNotNull(binderReceiver.get()); + } + + @Override + public void onDestroy() { + super.onDestroy(); + server.shutdownNow(); + handlerThread.quit(); + } + + /** A future representing a task submitted to a {@link Handler}. */ + private static class HandlerFuture implements ScheduledFuture { + + private final Duration delay; + private final SettableFuture delegate = SettableFuture.create(); + + HandlerFuture(Duration delay) { + this.delay = delay; + } + + @Override + public long getDelay(TimeUnit timeUnit) { + return timeUnit.convert(delay.toMillis(), TimeUnit.MILLISECONDS); + } + + @Override + public int compareTo(Delayed other) { + return Comparator + .comparingLong((Delayed delayed) -> delayed.getDelay(TimeUnit.MILLISECONDS)) + .compare(this, other); + } + + @Override + public boolean cancel(boolean mayInterruptIfRunning) { + return delegate.cancel(mayInterruptIfRunning); + } + + @Override + public boolean isCancelled() { + return delegate.isCancelled(); + } + + @Override + public boolean isDone() { + return delegate.isDone(); + } + + @Override + public V get() throws ExecutionException, InterruptedException { + return delegate.get(); + } + + @Override + public V get(long timeout, TimeUnit timeUnit) + throws ExecutionException, InterruptedException, TimeoutException { + return delegate.get(timeout, timeUnit); + } + + void complete(V result) { + delegate.set(result); + } + + void setException(Exception e) { + delegate.setException(e); + } + } + + /** + * Minimal implementation of a {@link ScheduledExecutorService} that delegates tasks to a + * {@link Handler}. Pending tasks can be forced to run via {@link #idleLooper()}. + */ + private class HandlerScheduledExecutorService implements ScheduledExecutorService { + + private Runnable asRunnableFor(HandlerFuture future, Runnable runnable) { + return () -> { + try { + runnable.run(); + future.complete(null); + } catch (Exception e) { + future.setException(e); + } + }; + } + private Runnable asRunnableFor(HandlerFuture future, Callable callable) { + return () -> { + try { + future.complete(callable.call()); + } catch (Exception e) { + future.setException(e); + } + }; + } + + @Override + public ScheduledFuture schedule(Runnable runnable, long l, TimeUnit timeUnit) { + long millis = timeUnit.toMillis(l); + HandlerFuture result = new HandlerFuture<>(Duration.ofMillis(millis)); + handler.postDelayed(asRunnableFor(result, runnable), millis); + return result; + } + + @Override + public ScheduledFuture schedule(Callable callable, long l, TimeUnit timeUnit) { + long millis = timeUnit.toMillis(l); + HandlerFuture result = new HandlerFuture<>(Duration.ofMillis(millis)); + handler.postDelayed(asRunnableFor(result, callable), millis); + return result; + } + + @Override + public ScheduledFuture scheduleAtFixedRate( + Runnable runnable, long delay, long period, TimeUnit timeUnit) { + return scheduleWithFixedDelay(runnable, delay, period, timeUnit); + } + + @Override + public ScheduledFuture scheduleWithFixedDelay( + Runnable runnable, long initialDelay, long delay, TimeUnit timeUnit) { + long initialDelayMillis = timeUnit.toMillis(initialDelay); + long periodMillis = timeUnit.toMillis(delay); + HandlerFuture result = new HandlerFuture<>(Duration.ofMillis(initialDelayMillis)); + + Runnable scheduledRunnable = new Runnable() { + @Override + public void run() { + try { + runnable.run(); + handler.postDelayed(this, periodMillis); + } catch (Exception e) { + result.setException(e); + } + } + }; + + handler.postDelayed(scheduledRunnable, initialDelayMillis); + return result; + } + + @Override + public void shutdown() { + handlerThread.quitSafely(); + } + + @Override + public List shutdownNow() { + handlerThread.quit(); + return ImmutableList.of(); + } + + @Override + public boolean isShutdown() { + return handlerThread.isAlive(); + } + + @Override + public boolean isTerminated() { + return handlerThread.isAlive(); + } + + @Override + public boolean awaitTermination(long l, TimeUnit timeUnit) { + idleLooper(); + return true; + } + + @Override + public Future submit(Callable callable) { + HandlerFuture result = new HandlerFuture<>(Duration.ZERO); + handler.post(asRunnableFor(result, callable)); + return result; + } + + @Override + public Future submit(Runnable runnable, T t) { + HandlerFuture result = new HandlerFuture<>(Duration.ZERO); + handler.post(asRunnableFor(result, () -> { + runnable.run(); + return t; + })); + return result; + } + + @Override + public Future submit(Runnable runnable) { + HandlerFuture result = new HandlerFuture<>(Duration.ZERO); + handler.post(asRunnableFor(result, runnable)); + return result; + } + + @Override + public List> invokeAll(Collection> collection) { + return collection.stream().map(this::submit).collect(Collectors.toList()); + } + + @Override + public List> invokeAll( + Collection> collection, long l, TimeUnit timeUnit) { + return collection + .stream() + .map(callable -> this.schedule(callable, l, timeUnit)) + .collect(Collectors.toList()); + } + + @Override + public T invokeAny(Collection> collection) + throws ExecutionException { + for (Callable callable : collection) { + try { + return submit(callable).get(); + } catch (Exception e) { + throw new ExecutionException(e); + } + } + throw new IllegalArgumentException(); + } + + @Override + public T invokeAny( + Collection> collection, long timeout, TimeUnit timeUnit) + throws ExecutionException { + for (Callable callable : collection) { + try { + return submit(callable).get(timeout, timeUnit); + } catch (Exception e) { + throw new ExecutionException(e); + } + } + throw new IllegalArgumentException(); + } + + @Override + public void execute(Runnable runnable) { + handler.post(runnable); + } + } + } +} From d47eb5b4d990432ea285cf674806698287e2cb32 Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Mon, 22 Jan 2024 10:55:08 -0800 Subject: [PATCH 05/10] Use AbstractExecutorService. --- .../binder/RobolectricBinderSecurityTest.java | 68 +------------------ 1 file changed, 2 insertions(+), 66 deletions(-) diff --git a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java index cb0887038ea..5ba0bc0b8af 100644 --- a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java +++ b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java @@ -57,6 +57,7 @@ import java.util.Collection; import java.util.Comparator; import java.util.List; +import java.util.concurrent.AbstractExecutorService; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.Callable; import java.util.concurrent.Delayed; @@ -314,7 +315,7 @@ void setException(Exception e) { * Minimal implementation of a {@link ScheduledExecutorService} that delegates tasks to a * {@link Handler}. Pending tasks can be forced to run via {@link #idleLooper()}. */ - private class HandlerScheduledExecutorService implements ScheduledExecutorService { + private class HandlerScheduledExecutorService extends AbstractExecutorService implements ScheduledExecutorService { private Runnable asRunnableFor(HandlerFuture future, Runnable runnable) { return () -> { @@ -408,71 +409,6 @@ public boolean awaitTermination(long l, TimeUnit timeUnit) { return true; } - @Override - public Future submit(Callable callable) { - HandlerFuture result = new HandlerFuture<>(Duration.ZERO); - handler.post(asRunnableFor(result, callable)); - return result; - } - - @Override - public Future submit(Runnable runnable, T t) { - HandlerFuture result = new HandlerFuture<>(Duration.ZERO); - handler.post(asRunnableFor(result, () -> { - runnable.run(); - return t; - })); - return result; - } - - @Override - public Future submit(Runnable runnable) { - HandlerFuture result = new HandlerFuture<>(Duration.ZERO); - handler.post(asRunnableFor(result, runnable)); - return result; - } - - @Override - public List> invokeAll(Collection> collection) { - return collection.stream().map(this::submit).collect(Collectors.toList()); - } - - @Override - public List> invokeAll( - Collection> collection, long l, TimeUnit timeUnit) { - return collection - .stream() - .map(callable -> this.schedule(callable, l, timeUnit)) - .collect(Collectors.toList()); - } - - @Override - public T invokeAny(Collection> collection) - throws ExecutionException { - for (Callable callable : collection) { - try { - return submit(callable).get(); - } catch (Exception e) { - throw new ExecutionException(e); - } - } - throw new IllegalArgumentException(); - } - - @Override - public T invokeAny( - Collection> collection, long timeout, TimeUnit timeUnit) - throws ExecutionException { - for (Callable callable : collection) { - try { - return submit(callable).get(timeout, timeUnit); - } catch (Exception e) { - throw new ExecutionException(e); - } - } - throw new IllegalArgumentException(); - } - @Override public void execute(Runnable runnable) { handler.post(runnable); From e60da766037a6909e8abacd3db97e02ccc76a3f5 Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Mon, 22 Jan 2024 11:19:10 -0800 Subject: [PATCH 06/10] Ran code through internal linter --- .../binder/RobolectricBinderSecurityTest.java | 156 +++++++++--------- 1 file changed, 76 insertions(+), 80 deletions(-) diff --git a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java index 5ba0bc0b8af..5cdde5745ae 100644 --- a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java +++ b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java @@ -19,13 +19,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.truth.Truth.assertThat; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; - -import com.google.common.collect.ImmutableList; -import com.google.common.util.concurrent.Futures; -import com.google.common.util.concurrent.ListenableFuture; -import com.google.common.util.concurrent.SettableFuture; -import com.google.common.util.concurrent.Uninterruptibles; -import com.google.protobuf.Empty; +import static org.robolectric.Shadows.shadowOf; import android.app.Application; import android.content.ComponentName; @@ -34,27 +28,29 @@ import android.os.HandlerThread; import android.os.IBinder; import android.os.Looper; - +import androidx.lifecycle.LifecycleService; +import androidx.test.core.app.ApplicationProvider; +import com.google.common.collect.ImmutableList; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; +import com.google.common.util.concurrent.Uninterruptibles; +import com.google.protobuf.Empty; import io.grpc.CallOptions; import io.grpc.ClientCall; import io.grpc.ManagedChannel; +import io.grpc.MethodDescriptor; +import io.grpc.Server; +import io.grpc.ServerCallHandler; +import io.grpc.ServerMethodDefinition; +import io.grpc.ServerServiceDefinition; import io.grpc.Status; import io.grpc.StatusRuntimeException; import io.grpc.protobuf.lite.ProtoLiteUtils; - -import androidx.lifecycle.LifecycleService; -import androidx.test.core.app.ApplicationProvider; - -import org.junit.After; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.robolectric.Robolectric; -import org.robolectric.RobolectricTestRunner; -import org.robolectric.android.controller.ServiceController; - +import io.grpc.stub.ClientCalls; +import io.grpc.stub.ServerCalls; import java.io.IOException; -import java.util.Collection; +import java.time.Duration; import java.util.Comparator; import java.util.List; import java.util.concurrent.AbstractExecutorService; @@ -62,23 +58,17 @@ import java.util.concurrent.Callable; import java.util.concurrent.Delayed; import java.util.concurrent.ExecutionException; -import java.util.concurrent.Future; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; -import java.time.Duration; -import java.util.stream.Collectors; - -import io.grpc.MethodDescriptor; -import io.grpc.Server; -import io.grpc.ServerCallHandler; -import io.grpc.ServerMethodDefinition; -import io.grpc.ServerServiceDefinition; -import io.grpc.stub.ClientCalls; -import io.grpc.stub.ServerCalls; - -import static org.robolectric.Shadows.shadowOf; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.Robolectric; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.android.controller.ServiceController; @RunWith(RobolectricTestRunner.class) public final class RobolectricBinderSecurityTest { @@ -97,11 +87,12 @@ public void setUp() { AndroidComponentAddress listenAddress = AndroidComponentAddress.forContext(service); ScheduledExecutorService executor = service.getExecutor(); - channel = BinderChannelBuilder.forAddress(listenAddress, context) - .executor(executor) - .scheduledExecutorService(executor) - .offloadExecutor(executor) - .build(); + channel = + BinderChannelBuilder.forAddress(listenAddress, context) + .executor(executor) + .scheduledExecutorService(executor) + .offloadExecutor(executor) + .build(); idleLoopers(); } @@ -137,8 +128,7 @@ private void idleLoopers() { private ListenableFuture makeCall() { ClientCall call = channel.newCall( - getMethodDescriptor(), - CallOptions.DEFAULT.withExecutor(service.getExecutor())); + getMethodDescriptor(), CallOptions.DEFAULT.withExecutor(service.getExecutor())); ListenableFuture responseFuture = ClientCalls.futureUnaryCall(call, Empty.getDefaultInstance()); @@ -189,29 +179,32 @@ public void onCreate() { }); ServerMethodDefinition methodDef = ServerMethodDefinition.create(methodDesc, callHandler); - ServerServiceDefinition def = ServerServiceDefinition.builder(SERVICE_NAME) - .addMethod(methodDef) - .build(); - - server = BinderServerBuilder.forAddress( - AndroidComponentAddress.forContext(this), - binderReceiver) - .addService(def) - .securityPolicy(ServerSecurityPolicy.newBuilder() - .servicePolicy(SERVICE_NAME, new AsyncSecurityPolicy() { - @Override - ListenableFuture checkAuthorizationAsync(int uid) { - return Futures.submitAsync(() -> { - SettableFuture status = SettableFuture.create(); - statusesToSet.add(status); - return status; - }, getExecutor()); - } - }) - .build()) - .executor(getExecutor()) - .scheduledExecutorService(getExecutor()) - .build(); + ServerServiceDefinition def = + ServerServiceDefinition.builder(SERVICE_NAME).addMethod(methodDef).build(); + + server = + BinderServerBuilder.forAddress(AndroidComponentAddress.forContext(this), binderReceiver) + .addService(def) + .securityPolicy( + ServerSecurityPolicy.newBuilder() + .servicePolicy( + SERVICE_NAME, + new AsyncSecurityPolicy() { + @Override + ListenableFuture checkAuthorizationAsync(int uid) { + return Futures.submitAsync( + () -> { + SettableFuture status = SettableFuture.create(); + statusesToSet.add(status); + return status; + }, + getExecutor()); + } + }) + .build()) + .executor(getExecutor()) + .scheduledExecutorService(getExecutor()) + .build(); try { server.start(); } catch (IOException e) { @@ -271,8 +264,8 @@ public long getDelay(TimeUnit timeUnit) { @Override public int compareTo(Delayed other) { - return Comparator - .comparingLong((Delayed delayed) -> delayed.getDelay(TimeUnit.MILLISECONDS)) + return Comparator.comparingLong( + (Delayed delayed) -> delayed.getDelay(TimeUnit.MILLISECONDS)) .compare(this, other); } @@ -312,10 +305,11 @@ void setException(Exception e) { } /** - * Minimal implementation of a {@link ScheduledExecutorService} that delegates tasks to a - * {@link Handler}. Pending tasks can be forced to run via {@link #idleLooper()}. + * Minimal implementation of a {@link ScheduledExecutorService} that delegates tasks to a {@link + * Handler}. Pending tasks can be forced to run via {@link #idleLooper()}. */ - private class HandlerScheduledExecutorService extends AbstractExecutorService implements ScheduledExecutorService { + private class HandlerScheduledExecutorService extends AbstractExecutorService + implements ScheduledExecutorService { private Runnable asRunnableFor(HandlerFuture future, Runnable runnable) { return () -> { @@ -327,6 +321,7 @@ private Runnable asRunnableFor(HandlerFuture future, Runnable runnable) { } }; } + private Runnable asRunnableFor(HandlerFuture future, Callable callable) { return () -> { try { @@ -366,17 +361,18 @@ public ScheduledFuture scheduleWithFixedDelay( long periodMillis = timeUnit.toMillis(delay); HandlerFuture result = new HandlerFuture<>(Duration.ofMillis(initialDelayMillis)); - Runnable scheduledRunnable = new Runnable() { - @Override - public void run() { - try { - runnable.run(); - handler.postDelayed(this, periodMillis); - } catch (Exception e) { - result.setException(e); - } - } - }; + Runnable scheduledRunnable = + new Runnable() { + @Override + public void run() { + try { + runnable.run(); + handler.postDelayed(this, periodMillis); + } catch (Exception e) { + result.setException(e); + } + } + }; handler.postDelayed(scheduledRunnable, initialDelayMillis); return result; From 6614bf870bb7d633ab5d790795ec852b19d43a2c Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Mon, 22 Jan 2024 14:21:37 -0800 Subject: [PATCH 07/10] Queue polling + idling --- .../binder/RobolectricBinderSecurityTest.java | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java index 5cdde5745ae..d15cf8877e6 100644 --- a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java +++ b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java @@ -34,7 +34,6 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; -import com.google.common.util.concurrent.Uninterruptibles; import com.google.protobuf.Empty; import io.grpc.CallOptions; import io.grpc.ClientCall; @@ -62,6 +61,7 @@ import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import javax.annotation.Nullable; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -120,11 +120,6 @@ public void testAsyncServerSecurityPolicy_allowed_returnsOkStatus() throws Excep assertThat(Futures.getDone(status).getCode()).isEqualTo(Status.Code.OK); } - private void idleLoopers() { - service.idleLooper(); - shadowOf(Looper.getMainLooper()).idle(); - } - private ListenableFuture makeCall() { ClientCall call = channel.newCall( @@ -141,6 +136,15 @@ private ListenableFuture makeCall() { directExecutor()); } + private void idleLoopers() { + idleLoopers(service); + } + + private static void idleLoopers(SomeService service) { + service.idleLooper(); + shadowOf(Looper.getMainLooper()).idle(); + } + private static MethodDescriptor getMethodDescriptor() { MethodDescriptor.Marshaller marshaller = ProtoLiteUtils.marshaller(Empty.getDefaultInstance()); @@ -231,7 +235,14 @@ void idleLooper() { } void setSecurityPolicyStatusWhenReady(Status status) { - Uninterruptibles.takeUninterruptibly(statusesToSet).set(status); + @Nullable SettableFuture future = statusesToSet.poll(); + while (future == null) { + // It's possible that either the test thread or the gRPC thread has posted tasks to each + // other's executor. Keep idling until the future is available. + idleLoopers(this); + future = statusesToSet.poll(); + } + future.set(status); } @Override From 8183ca4b9f1223aca3ce4262d2c6b7aa44aae3ec Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Mon, 22 Jan 2024 14:32:36 -0800 Subject: [PATCH 08/10] Use only main thread. New test case for failed future. --- .../binder/RobolectricBinderSecurityTest.java | 60 +++++++++---------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java index d15cf8877e6..48f87594cf7 100644 --- a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java +++ b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java @@ -25,7 +25,6 @@ import android.content.ComponentName; import android.content.Intent; import android.os.Handler; -import android.os.HandlerThread; import android.os.IBinder; import android.os.Looper; import androidx.lifecycle.LifecycleService; @@ -111,6 +110,15 @@ public void testAsyncServerSecurityPolicy_failed_returnsFailureStatus() throws E assertThat(Futures.getDone(status).getCode()).isEqualTo(Status.Code.ALREADY_EXISTS); } + @Test + public void testAsyncServerSecurityPolicy_failedFuture_failsWithCodeInternal() throws Exception { + ListenableFuture status = makeCall(); + service.setSecurityPolicyFailed(new IllegalStateException("oops")); + idleLoopers(); + + assertThat(Futures.getDone(status).getCode()).isEqualTo(Status.Code.INTERNAL); + } + @Test public void testAsyncServerSecurityPolicy_allowed_returnsOkStatus() throws Exception { ListenableFuture status = makeCall(); @@ -136,12 +144,7 @@ private ListenableFuture makeCall() { directExecutor()); } - private void idleLoopers() { - idleLoopers(service); - } - - private static void idleLoopers(SomeService service) { - service.idleLooper(); + private static void idleLoopers() { shadowOf(Looper.getMainLooper()).idle(); } @@ -162,17 +165,12 @@ private static class SomeService extends LifecycleService { private final ArrayBlockingQueue> statusesToSet = new ArrayBlockingQueue<>(128); private Server server; - private HandlerThread handlerThread; - private Handler handler; private final ScheduledExecutorService scheduledExecutorService = new HandlerScheduledExecutorService(); @Override public void onCreate() { super.onCreate(); - handlerThread = new HandlerThread("test_handler_thread"); - handlerThread.start(); - handler = new Handler(handlerThread.getLooper()); MethodDescriptor methodDesc = getMethodDescriptor(); ServerCallHandler callHandler = @@ -224,25 +222,29 @@ ListenableFuture checkAuthorizationAsync(int uid) { /** * Returns an {@link ScheduledExecutorService} under which all of the gRPC computations run. The - * execution of any pending tasks on this executor can be triggered via {@link #idleLooper()}. + * execution of any pending tasks on this executor can be triggered via {@link #idleLoopers()}. */ ScheduledExecutorService getExecutor() { return scheduledExecutorService; } - void idleLooper() { - shadowOf(handlerThread.getLooper()).idle(); + void setSecurityPolicyStatusWhenReady(Status status) { + getNextEnqueuedStatus().set(status); } - void setSecurityPolicyStatusWhenReady(Status status) { + void setSecurityPolicyFailed(Exception e) { + getNextEnqueuedStatus().setException(e); + } + + private SettableFuture getNextEnqueuedStatus() { @Nullable SettableFuture future = statusesToSet.poll(); while (future == null) { // It's possible that either the test thread or the gRPC thread has posted tasks to each // other's executor. Keep idling until the future is available. - idleLoopers(this); + idleLoopers(); future = statusesToSet.poll(); } - future.set(status); + return checkNotNull(future); } @Override @@ -255,7 +257,6 @@ public IBinder onBind(Intent intent) { public void onDestroy() { super.onDestroy(); server.shutdownNow(); - handlerThread.quit(); } /** A future representing a task submitted to a {@link Handler}. */ @@ -317,12 +318,14 @@ void setException(Exception e) { /** * Minimal implementation of a {@link ScheduledExecutorService} that delegates tasks to a {@link - * Handler}. Pending tasks can be forced to run via {@link #idleLooper()}. + * Handler}. Pending tasks can be forced to run via {@link + * org.robolectric.shadows.ShadowLooper#idle()}. */ - private class HandlerScheduledExecutorService extends AbstractExecutorService + private static class HandlerScheduledExecutorService extends AbstractExecutorService implements ScheduledExecutorService { + private final Handler handler = new Handler(Looper.getMainLooper()); - private Runnable asRunnableFor(HandlerFuture future, Runnable runnable) { + private static Runnable asRunnableFor(HandlerFuture future, Runnable runnable) { return () -> { try { runnable.run(); @@ -333,7 +336,7 @@ private Runnable asRunnableFor(HandlerFuture future, Runnable runnable) { }; } - private Runnable asRunnableFor(HandlerFuture future, Callable callable) { + private static Runnable asRunnableFor(HandlerFuture future, Callable callable) { return () -> { try { future.complete(callable.call()); @@ -390,29 +393,26 @@ public void run() { } @Override - public void shutdown() { - handlerThread.quitSafely(); - } + public void shutdown() {} @Override public List shutdownNow() { - handlerThread.quit(); return ImmutableList.of(); } @Override public boolean isShutdown() { - return handlerThread.isAlive(); + return false; } @Override public boolean isTerminated() { - return handlerThread.isAlive(); + return false; } @Override public boolean awaitTermination(long l, TimeUnit timeUnit) { - idleLooper(); + idleLoopers(); return true; } From 2badc3f65bd21436108e504636b13e4316a2ac5d Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Tue, 23 Jan 2024 08:24:39 -0800 Subject: [PATCH 09/10] Rephrase comment --- .../java/io/grpc/binder/RobolectricBinderSecurityTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java index 48f87594cf7..593216bc173 100644 --- a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java +++ b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java @@ -239,8 +239,7 @@ void setSecurityPolicyFailed(Exception e) { private SettableFuture getNextEnqueuedStatus() { @Nullable SettableFuture future = statusesToSet.poll(); while (future == null) { - // It's possible that either the test thread or the gRPC thread has posted tasks to each - // other's executor. Keep idling until the future is available. + // Keep idling until the future is available. idleLoopers(); future = statusesToSet.poll(); } From f1e24e5246d203b13f010e01c298ea45ed73d5e3 Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Tue, 23 Jan 2024 10:01:30 -0800 Subject: [PATCH 10/10] Fix broken test after merge. --- .../test/java/io/grpc/binder/RobolectricBinderSecurityTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java index 593216bc173..ba73cc95578 100644 --- a/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java +++ b/binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java @@ -193,7 +193,7 @@ public void onCreate() { SERVICE_NAME, new AsyncSecurityPolicy() { @Override - ListenableFuture checkAuthorizationAsync(int uid) { + public ListenableFuture checkAuthorizationAsync(int uid) { return Futures.submitAsync( () -> { SettableFuture status = SettableFuture.create();