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

Author: olavloite

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

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

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());
+ }
+ }
 }