From 5ceb522361f05191ea202bbc28d84cdbc54838ca Mon Sep 17 00:00:00 2001
From: Olav Loite <koloite@gmail.com>
Date: Wed, 15 May 2019 08:56:17 +0200
Subject: [PATCH 1/2] do not hand out a closed Spanner instance

SpannerOptions caches any Spanner instance that has been created, and
hands this cached instance out to all subsequent calls to
SpannerOptions.getService(). This also included closed Spanner
instances. The getService() method now returns an error if the Spanner
instance has already been closed.
---
 .../com/google/cloud/spanner/Spanner.java     |  3 ++
 .../com/google/cloud/spanner/SpannerImpl.java |  5 ++
 .../google/cloud/spanner/SpannerOptions.java  | 30 ++++++++++++
 .../cloud/spanner/spi/v1/GapicSpannerRpc.java |  7 +++
 .../cloud/spanner/spi/v1/SpannerRpc.java      |  2 +
 .../google/cloud/spanner/SpannerImplTest.java | 47 +++++++++++++++++++
 .../spanner/spi/v1/GapicSpannerRpcTest.java   |  1 +
 7 files changed, 95 insertions(+)

diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
index 75c062f96811..0c6bec4ea830 100644
--- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
+++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
@@ -105,4 +105,7 @@ public interface Spanner extends Service<SpannerOptions>, AutoCloseable {
    */
   @Override
   void close();
+
+  /** @return <code>true</code> if this {@link Spanner} object is closed. */
+  boolean isClosed();
 }
diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java
index 5e60686c7e5b..9ee941584c11 100644
--- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java
+++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java
@@ -291,6 +291,11 @@ public void close() {
     }
   }
 
+  @Override
+  public boolean isClosed() {
+    return spannerIsClosed;
+  }
+
   /**
    * Checks that the current context is still valid, throwing a CANCELLED or DEADLINE_EXCEEDED error
    * if not.
diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
index b5457f814d26..0ddee6b5d9a1 100644
--- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
+++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java
@@ -298,6 +298,36 @@ protected SpannerRpc getSpannerRpcV1() {
     return (SpannerRpc) getRpc();
   }
 
+  /**
+   * Returns a {@link Spanner} service object. Note that only the first call to this method will
+   * create a new instance, and all subsequent calls to this method will return the same instance.
+   * Calling this method after the {@link Spanner} instance has been closed will cause an exception.
+   */
+  @Override
+  public Spanner getService() {
+    Spanner spanner = super.getService();
+    if (spanner.isClosed()) {
+      throw new IllegalStateException(
+          "The Spanner service of this SpannerOptions has been closed. Create a new SpannerOptions instance and call getService() on the new instance instead.");
+    }
+    return spanner;
+  }
+
+  /**
+   * Returns a {@link SpannerRpc} object. Note that only the first call to this method will create a
+   * new instance, and all subsequent calls to this method will return the same instance. Calling
+   * this method after the instance has been closed will cause an exception.
+   */
+  @Override
+  public ServiceRpc getRpc() {
+    SpannerRpc rpc = (SpannerRpc) super.getRpc();
+    if (rpc.isClosed()) {
+      throw new IllegalStateException(
+          "The Spanner service of this SpannerOptions has been closed. Create a new SpannerOptions instance and call getRpc() on the new instance instead.");
+    }
+    return rpc;
+  }
+
   @SuppressWarnings("unchecked")
   @Override
   public Builder toBuilder() {
diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
index 4aaf8e1e6298..8b57dd7c2851 100644
--- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
+++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
@@ -168,6 +168,7 @@ private synchronized void shutdown() {
   private static final int DEFAULT_PERIOD_SECONDS = 10;
 
   private final ManagedInstantiatingExecutorProvider executorProvider;
+  private boolean rpcIsClosed;
   private final SpannerStub spannerStub;
   private final InstanceAdminStub instanceAdminStub;
   private final DatabaseAdminStub databaseAdminStub;
@@ -631,6 +632,7 @@ private GrpcCallContext newCallContext(@Nullable Map<Option, ?> options, String
 
   @Override
   public void shutdown() {
+    this.rpcIsClosed = true;
     this.spannerStub.close();
     this.instanceAdminStub.close();
     this.databaseAdminStub.close();
@@ -638,6 +640,11 @@ public void shutdown() {
     this.executorProvider.shutdown();
   }
 
+  @Override
+  public boolean isClosed() {
+    return rpcIsClosed;
+  }
+
   /**
    * A {@code ResponseObserver} that exposes the {@code StreamController} and delegates callbacks to
    * the {@link ResultStreamConsumer}.
diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java
index 500f369f67a8..593ba3c5ec06 100644
--- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java
+++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java
@@ -233,4 +233,6 @@ PartitionResponse partitionRead(PartitionReadRequest request, @Nullable Map<Opti
       throws SpannerException;
 
   public void shutdown();
+
+  boolean isClosed();
 }
diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java
index 8fe6e5c91d7b..7a6e1b0faefc 100644
--- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java
+++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java
@@ -21,6 +21,8 @@
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.fail;
 
+import com.google.cloud.NoCredentials;
+import com.google.cloud.ServiceRpc;
 import com.google.cloud.grpc.GrpcTransportOptions;
 import com.google.cloud.spanner.spi.v1.SpannerRpc;
 import java.util.HashMap;
@@ -181,4 +183,49 @@ public Void call() throws Exception {
           }
         });
   }
+
+  @Test
+  public void testSpannerClosed() throws InterruptedException {
+    SpannerOptions options = createSpannerOptions();
+    Spanner spanner1 = options.getService();
+    Spanner spanner2 = options.getService();
+    ServiceRpc rpc1 = options.getRpc();
+    ServiceRpc rpc2 = options.getRpc();
+    // The SpannerOptions object should return the same instance.
+    assertThat(spanner1 == spanner2, is(true));
+    assertThat(rpc1 == rpc2, is(true));
+    spanner1.close();
+    // The SpannerOptions should now no longer be valid.
+    try {
+      options.getService();
+      fail("missing expected exception");
+    } catch (IllegalStateException e) {
+      // This is the expected exception.
+    }
+    // The SpannerOptions should now no longer be valid.
+    try {
+      options.getRpc();
+      fail("missing expected exception");
+    } catch (IllegalStateException e) {
+      // This is the expected exception.
+    }
+    // Creating a copy of the SpannerOptions should result in new instances.
+    options = options.toBuilder().build();
+    spanner1 = options.getService();
+    rpc1 = options.getRpc();
+    assertThat(spanner1 == spanner2, is(false));
+    assertThat(rpc1 == rpc2, is(false));
+    spanner2 = options.getService();
+    rpc2 = options.getRpc();
+    assertThat(spanner1 == spanner2, is(true));
+    assertThat(rpc1 == rpc2, is(true));
+    spanner1.close();
+  }
+
+  private SpannerOptions createSpannerOptions() {
+    return SpannerOptions.newBuilder()
+        .setProjectId("[PROJECT]")
+        .setCredentials(NoCredentials.getInstance())
+        .build();
+  }
 }
diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java
index bbf53b0cf0cc..75f49d451dcb 100644
--- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java
+++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java
@@ -167,6 +167,7 @@ public void testCloseAllThreadsWhenClosingSpanner() throws InterruptedException
         DatabaseAdminClient databaseAdminClient = spanner.getDatabaseAdminClient();
         databaseAdminClient.getDatabase("projects/[PROJECT]/instances/[INSTANCE]", "[DATABASE]");
       }
+
       // Now close the Spanner instance and check whether the threads are shutdown or not.
       spanner.close();
       // Wait for up to two seconds to allow the threads to actually shutdown.

From d2c17465df0b067529bb74b001d111cd9a7fd596 Mon Sep 17 00:00:00 2001
From: Olav Loite <koloite@gmail.com>
Date: Wed, 15 May 2019 16:44:35 +0200
Subject: [PATCH 2/2] fix small merge error

---
 .../com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java     | 1 -
 1 file changed, 1 deletion(-)

diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java
index 75f49d451dcb..bbf53b0cf0cc 100644
--- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java
+++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java
@@ -167,7 +167,6 @@ public void testCloseAllThreadsWhenClosingSpanner() throws InterruptedException
         DatabaseAdminClient databaseAdminClient = spanner.getDatabaseAdminClient();
         databaseAdminClient.getDatabase("projects/[PROJECT]/instances/[INSTANCE]", "[DATABASE]");
       }
-
       // Now close the Spanner instance and check whether the threads are shutdown or not.
       spanner.close();
       // Wait for up to two seconds to allow the threads to actually shutdown.