From 60fb986f8b758a65e20c5315faf85fc0a935d0cc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= <koloite@gmail.com>
Date: Fri, 2 Oct 2020 01:42:39 +0200
Subject: [PATCH] fix: ResultSet#close() should not throw exceptions from
 session creation (#487)

* fix: ResultSet#close() should not throw exceptions from session creation

If a client application requested a session from the session pool, and that request
required a new BatchCreateSessions request that would fail, then the error from the
BatchCreateSessions RPC would be propagated to both the ReadContext#executeQuery(..)
method as well as the ResultSet#close() method. The latter should not happen, as the
close method should silently ignore any errors from the session creation.

* docs: clear up comment
---
 .../cloud/spanner/ForwardingResultSet.java    | 10 ++++++-
 .../com/google/cloud/spanner/SessionPool.java | 19 +++++++++++--
 .../cloud/spanner/DatabaseClientImplTest.java | 27 +++++++++++++++++++
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/ForwardingResultSet.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/ForwardingResultSet.java
index 4cc0ab9b9e..7d97fad162 100644
--- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/ForwardingResultSet.java
+++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/ForwardingResultSet.java
@@ -61,7 +61,15 @@ public Struct getCurrentRowAsStruct() {
 
   @Override
   public void close() {
-    delegate.get().close();
+    ResultSet rs;
+    try {
+      rs = delegate.get();
+    } catch (Exception e) {
+      // Ignore any exceptions when getting the underlying result set, as that means that there is
+      // nothing that needs to be closed.
+      return;
+    }
+    rs.close();
   }
 
   @Override
diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java
index 90e399fad6..2512024117 100644
--- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java
+++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java
@@ -1255,7 +1255,10 @@ public void close() {
         leakedException = null;
         checkedOutSessions.remove(this);
       }
-      get().close();
+      PooledSession delegate = getOrNull();
+      if (delegate != null) {
+        delegate.close();
+      }
     }
 
     @Override
@@ -1264,7 +1267,19 @@ public ApiFuture<Empty> asyncClose() {
         leakedException = null;
         checkedOutSessions.remove(this);
       }
-      return get().asyncClose();
+      PooledSession delegate = getOrNull();
+      if (delegate != null) {
+        return delegate.asyncClose();
+      }
+      return ApiFutures.immediateFuture(Empty.getDefaultInstance());
+    }
+
+    private PooledSession getOrNull() {
+      try {
+        return get();
+      } catch (Throwable t) {
+        return null;
+      }
     }
 
     @Override
diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
index bf425556b4..8775fb1b18 100644
--- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
+++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
@@ -1599,4 +1599,31 @@ public Long run(TransactionContext transaction) throws Exception {
               }
             });
   }
+
+  @Test
+  public void testBatchCreateSessionsFailure_shouldNotPropagateToCloseMethod() {
+    try {
+      // Simulate session creation failures on the backend.
+      mockSpanner.setBatchCreateSessionsExecutionTime(
+          SimulatedExecutionTime.ofStickyException(Status.RESOURCE_EXHAUSTED.asRuntimeException()));
+      DatabaseClient client =
+          spannerWithEmptySessionPool.getDatabaseClient(
+              DatabaseId.of(TEST_PROJECT, TEST_INSTANCE, TEST_DATABASE));
+      // This will not cause any failure as getting a session from the pool is guaranteed to be
+      // non-blocking, and any exceptions will be delayed until actual query execution.
+      ResultSet rs = client.singleUse().executeQuery(SELECT1);
+      try {
+        while (rs.next()) {
+          fail("Missing expected exception");
+        }
+      } catch (SpannerException e) {
+        assertThat(e.getErrorCode()).isEqualTo(ErrorCode.RESOURCE_EXHAUSTED);
+      } finally {
+        // This should not cause any failures.
+        rs.close();
+      }
+    } finally {
+      mockSpanner.setBatchCreateSessionsExecutionTime(SimulatedExecutionTime.none());
+    }
+  }
 }