From 2dfae40f05454884de52524e4bbde80c0824873e Mon Sep 17 00:00:00 2001 From: Vadym Matsishevskyi <25311427+vam-google@users.noreply.github.com> Date: Tue, 24 Sep 2019 10:05:40 -0700 Subject: [PATCH] Fix Race condition when shutting down executor/transport channel (#787) This fixes the customer issue ("transitively"): https://github.com/googleapis/gax-java/issues/785 This PR only changes the order of shutdowns (first transportChannel and only then the Executor). This the most lightweight fix we can make, which should fix the issue. We would like to avoid enforcing `awaitTermination()` on `shutdown()` because that would convert non-blocking call (`shutdown()`) to a blocking one with great potential of having non-desirable side-effects. Also this may potentially require surface changes on gax (potentially requiring major version bump on gax), because currently `shutdown()` is widely used, it is declared in a public interface and is explicitly specified as non-blocking operation. --- dependencies.properties | 1 + gax-grpc/BUILD.bazel | 1 + gax-httpjson/BUILD.bazel | 1 + gax/BUILD.bazel | 1 + .../com/google/api/gax/rpc/ClientContext.java | 17 ++++++++-------- .../google/api/gax/rpc/ClientContextTest.java | 20 ++++++++++++++----- 6 files changed, 28 insertions(+), 13 deletions(-) diff --git a/dependencies.properties b/dependencies.properties index bba29b780..0a000e652 100644 --- a/dependencies.properties +++ b/dependencies.properties @@ -76,5 +76,6 @@ maven.junit_junit=junit:junit:4.12 maven.org_mockito_mockito_core=org.mockito:mockito-core:2.21.0 maven.org_hamcrest_hamcrest_core=org.hamcrest:hamcrest-core:1.3 maven.com_google_truth_truth=com.google.truth:truth:0.44 +maven.com_googlecode_java_diff_utils_diffutils=com.googlecode.java-diff-utils:diffutils:1.3.0 maven.net_bytebuddy_byte_buddy=net.bytebuddy:byte-buddy:1.8.15 maven.org_objenesis_objenesis=org.objenesis:objenesis:2.6 diff --git a/gax-grpc/BUILD.bazel b/gax-grpc/BUILD.bazel index 2ec5b4b77..46a322255 100644 --- a/gax-grpc/BUILD.bazel +++ b/gax-grpc/BUILD.bazel @@ -37,6 +37,7 @@ _TEST_COMPILE_DEPS = [ "@com_google_api_grpc_grpc_google_common_protos//jar", "@org_apache_commons_commons_lang3//jar", "//gax:gax_testlib", + "@com_googlecode_java_diff_utils_diffutils//jar", ] java_library( diff --git a/gax-httpjson/BUILD.bazel b/gax-httpjson/BUILD.bazel index 8037511bd..bba76e8ac 100644 --- a/gax-httpjson/BUILD.bazel +++ b/gax-httpjson/BUILD.bazel @@ -26,6 +26,7 @@ _TEST_COMPILE_DEPS = [ "@org_mockito_mockito_core//jar", "@com_google_truth_truth//jar", "//gax:gax_testlib", + "@com_googlecode_java_diff_utils_diffutils//jar", ] java_library( diff --git a/gax/BUILD.bazel b/gax/BUILD.bazel index e93a0e898..ef99df10e 100644 --- a/gax/BUILD.bazel +++ b/gax/BUILD.bazel @@ -31,6 +31,7 @@ _TEST_COMPILE_DEPS = [ "@org_hamcrest_hamcrest_core//jar", "@net_bytebuddy_byte_buddy//jar", "@org_objenesis_objenesis//jar", + "@com_googlecode_java_diff_utils_diffutils//jar", ] java_library( diff --git a/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java b/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java index c5471b04c..391f3d959 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java +++ b/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java @@ -127,15 +127,10 @@ public static ClientContext create(ClientSettings settings) throws IOException { * settings. */ public static ClientContext create(StubSettings settings) throws IOException { - ImmutableList.Builder backgroundResources = ImmutableList.builder(); - ApiClock clock = settings.getClock(); ExecutorProvider executorProvider = settings.getExecutorProvider(); final ScheduledExecutorService executor = executorProvider.getExecutor(); - if (executorProvider.shouldAutoClose()) { - backgroundResources.add(new ExecutorAsBackgroundResource(executor)); - } Credentials credentials = settings.getCredentialsProvider().getCredentials(); @@ -158,9 +153,6 @@ public static ClientContext create(StubSettings settings) throws IOException { transportChannelProvider = transportChannelProvider.withCredentials(credentials); } TransportChannel transportChannel = transportChannelProvider.getTransportChannel(); - if (transportChannelProvider.shouldAutoClose()) { - backgroundResources.add(transportChannel); - } ApiCallContext defaultCallContext = transportChannel.getEmptyCallContext().withTransportChannel(transportChannel); @@ -185,6 +177,15 @@ public static ClientContext create(StubSettings settings) throws IOException { watchdog = watchdogProvider.getWatchdog(); } + ImmutableList.Builder backgroundResources = ImmutableList.builder(); + + if (transportChannelProvider.shouldAutoClose()) { + backgroundResources.add(transportChannel); + } + if (executorProvider.shouldAutoClose()) { + backgroundResources.add(new ExecutorAsBackgroundResource(executor)); + } + return newBuilder() .setBackgroundResources(backgroundResources.build()) .setExecutor(executor) diff --git a/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java b/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java index 1eadb8de4..2d0830a5e 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java @@ -40,6 +40,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.truth.Truth; import java.io.IOException; +import java.util.List; import java.util.Map; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledThreadPoolExecutor; @@ -270,11 +271,20 @@ private void runTest( Truth.assertThat(executor.shutdownCalled).isFalse(); Truth.assertThat(transportChannel.isShutdown()).isFalse(); - for (BackgroundResource backgroundResource : clientContext.getBackgroundResources()) { - backgroundResource.shutdown(); + List resources = clientContext.getBackgroundResources(); + + if (!resources.isEmpty()) { + // This is slightly too implementation-specific, but we need to ensure that executor is shut + // down after the transportChannel: https://github.com/googleapis/gax-java/issues/785 + Truth.assertThat(resources.size()).isEqualTo(2); + Truth.assertThat(transportChannel.isShutdown()).isNotEqualTo(shouldAutoClose); + Truth.assertThat(executor.shutdownCalled).isNotEqualTo(shouldAutoClose); + resources.get(0).shutdown(); + Truth.assertThat(transportChannel.isShutdown()).isEqualTo(shouldAutoClose); + Truth.assertThat(executor.shutdownCalled).isNotEqualTo(shouldAutoClose); + resources.get(1).shutdown(); + Truth.assertThat(transportChannel.isShutdown()).isEqualTo(shouldAutoClose); + Truth.assertThat(executor.shutdownCalled).isEqualTo(shouldAutoClose); } - - Truth.assertThat(executor.shutdownCalled).isEqualTo(shouldAutoClose); - Truth.assertThat(transportChannel.isShutdown()).isEqualTo(shouldAutoClose); } }