Skip to content
Permalink
Browse files
fix: ResultSet#close() should not throw exceptions from session creat…
…ion (#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
  • Loading branch information
olavloite committed Oct 1, 2020
1 parent 20bce56 commit 60fb986f8b758a65e20c5315faf85fc0a935d0cc
@@ -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
@@ -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
@@ -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());
}
}
}

0 comments on commit 60fb986

Please sign in to comment.