Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Oct 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -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
Expand Down
Expand Up @@ -1255,7 +1255,10 @@ public void close() {
leakedException = null;
checkedOutSessions.remove(this);
}
get().close();
PooledSession delegate = getOrNull();
if (delegate != null) {
delegate.close();
}
}

@Override
Expand All @@ -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
Expand Down
Expand Up @@ -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());
}
}
}