Fix Race condition when shutting down executor/transport channel (#787)

This fixes the customer issue ("transitively"): #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

@@ -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

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(

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(

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(

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<BackgroundResource> 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<BackgroundResource> backgroundResources = ImmutableList.builder();
+
+    if (transportChannelProvider.shouldAutoClose()) {
+      backgroundResources.add(transportChannel);
+    }
+    if (executorProvider.shouldAutoClose()) {
+      backgroundResources.add(new ExecutorAsBackgroundResource(executor));
+    }
+
     return newBuilder()
         .setBackgroundResources(backgroundResources.build())
         .setExecutor(executor)

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<BackgroundResource> 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);
   }
 }