File tree

7 files changed

+111
-31
lines changed

7 files changed

+111
-31
lines changed
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.google.cloud.Timestamp;
2020
import com.google.cloud.spanner.SessionPool.PooledSession;
21+
import com.google.cloud.spanner.SpannerImpl.ClosedException;
2122
import com.google.common.annotations.VisibleForTesting;
2223
import com.google.common.base.Function;
2324
import com.google.common.util.concurrent.ListenableFuture;
@@ -225,7 +226,7 @@ private <T> T runWithSessionRetry(SessionMode mode, Function<Session, T> callabl
225226
}
226227
}
227228

228-
ListenableFuture<Void> closeAsync() {
229-
return pool.closeAsync();
229+
ListenableFuture<Void> closeAsync(ClosedException closedException) {
230+
return pool.closeAsync(closedException);
230231
}
231232
}
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import com.google.cloud.spanner.Options.ReadOption;
4343
import com.google.cloud.spanner.SessionClient.SessionConsumer;
4444
import com.google.cloud.spanner.SpannerException.ResourceNotFoundException;
45+
import com.google.cloud.spanner.SpannerImpl.ClosedException;
4546
import com.google.common.annotations.VisibleForTesting;
4647
import com.google.common.base.Function;
4748
import com.google.common.base.MoreObjects;
@@ -1123,6 +1124,9 @@ private static enum Position {
11231124
@GuardedBy("lock")
11241125
private SettableFuture<Void> closureFuture;
11251126

1127+
@GuardedBy("lock")
1128+
private ClosedException closedException;
1129+
11261130
@GuardedBy("lock")
11271131
private ResourceNotFoundException resourceNotFoundException;
11281132

@@ -1428,7 +1432,7 @@ PooledSession getReadSession() throws SpannerException {
14281432
synchronized (lock) {
14291433
if (closureFuture != null) {
14301434
span.addAnnotation("Pool has been closed");
1431-
throw new IllegalStateException("Pool has been closed");
1435+
throw new IllegalStateException("Pool has been closed", closedException);
14321436
}
14331437
if (resourceNotFoundException != null) {
14341438
span.addAnnotation("Database has been deleted");
@@ -1497,7 +1501,7 @@ PooledSession getReadWriteSession() {
14971501
synchronized (lock) {
14981502
if (closureFuture != null) {
14991503
span.addAnnotation("Pool has been closed");
1500-
throw new IllegalStateException("Pool has been closed");
1504+
throw new IllegalStateException("Pool has been closed", closedException);
15011505
}
15021506
if (resourceNotFoundException != null) {
15031507
span.addAnnotation("Database has been deleted");
@@ -1761,12 +1765,13 @@ private void decrementPendingClosures(int count) {
17611765
* #getReadWriteSession()} will start throwing {@code IllegalStateException}. The returned future
17621766
* blocks till all the sessions created in this pool have been closed.
17631767
*/
1764-
ListenableFuture<Void> closeAsync() {
1768+
ListenableFuture<Void> closeAsync(ClosedException closedException) {
17651769
ListenableFuture<Void> retFuture = null;
17661770
synchronized (lock) {
17671771
if (closureFuture != null) {
1768-
throw new IllegalStateException("Close has already been invoked");
1772+
throw new IllegalStateException("Close has already been invoked", this.closedException);
17691773
}
1774+
this.closedException = closedException;
17701775
// Fail all pending waiters.
17711776
Waiter waiter = readWaiters.poll();
17721777
while (waiter != null) {
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
import java.util.logging.Logger;
4747
import javax.annotation.Nullable;
4848
import javax.annotation.concurrent.GuardedBy;
49+
import org.threeten.bp.Instant;
4950

5051
/** Default implementation of the Cloud Spanner interface. */
5152
class SpannerImpl extends BaseService<SpannerOptions> implements Spanner {
@@ -94,8 +95,22 @@ private static String nextDatabaseClientId(DatabaseId databaseId) {
9495
private final DatabaseAdminClient dbAdminClient;
9596
private final InstanceAdminClient instanceClient;
9697

98+
/**
99+
* Exception class used to track the stack trace at the point when a Spanner instance is closed.
100+
* This exception will be thrown if a user tries to use any resources that were returned by this
101+
* Spanner instance after the instance has been closed. This makes it easier to track down the
102+
* code that (accidently) closed the Spanner instance.
103+
*/
104+
static final class ClosedException extends RuntimeException {
105+
private static final long serialVersionUID = 1451131180314064914L;
106+
107+
ClosedException() {
108+
super("Spanner client was closed at " + Instant.now());
109+
}
110+
}
111+
97112
@GuardedBy("this")
98-
private boolean spannerIsClosed = false;
113+
private ClosedException closedException;
99114

100115
@VisibleForTesting
101116
SpannerImpl(SpannerRpc gapicRpc, SpannerOptions options) {
@@ -131,9 +146,17 @@ SessionImpl sessionWithId(String name) {
131146
return getSessionClient(id.getDatabaseId()).sessionWithId(name);
132147
}
133148

149+
void checkClosed() {
150+
synchronized (this) {
151+
if (closedException != null) {
152+
throw new IllegalStateException("Cloud Spanner client has been closed", closedException);
153+
}
154+
}
155+
}
156+
134157
SessionClient getSessionClient(DatabaseId db) {
135158
synchronized (this) {
136-
Preconditions.checkState(!spannerIsClosed, "Cloud Spanner client has been closed");
159+
checkClosed();
137160
if (sessionClients.containsKey(db)) {
138161
return sessionClients.get(db);
139162
} else {
@@ -161,7 +184,7 @@ public InstanceAdminClient getInstanceAdminClient() {
161184
@Override
162185
public DatabaseClient getDatabaseClient(DatabaseId db) {
163186
synchronized (this) {
164-
Preconditions.checkState(!spannerIsClosed, "Cloud Spanner client has been closed");
187+
checkClosed();
165188
if (dbClients.containsKey(db) && !dbClients.get(db).pool.isValid()) {
166189
// Move the invalidated client to a separate list, so we can close it together with the
167190
// other database clients when the Spanner instance is closed.
@@ -206,12 +229,12 @@ public void close() {
206229
void close(long timeout, TimeUnit unit) {
207230
List<ListenableFuture<Void>> closureFutures = null;
208231
synchronized (this) {
209-
Preconditions.checkState(!spannerIsClosed, "Cloud Spanner client has been closed");
210-
spannerIsClosed = true;
232+
checkClosed();
233+
closedException = new ClosedException();
211234
closureFutures = new ArrayList<>();
212235
invalidatedDbClients.addAll(dbClients.values());
213236
for (DatabaseClientImpl dbClient : invalidatedDbClients) {
214-
closureFutures.add(dbClient.closeAsync());
237+
closureFutures.add(dbClient.closeAsync(closedException));
215238
}
216239
dbClients.clear();
217240
}
@@ -234,7 +257,9 @@ void close(long timeout, TimeUnit unit) {
234257

235258
@Override
236259
public boolean isClosed() {
237-
return spannerIsClosed;
260+
synchronized (this) {
261+
return closedException != null;
262+
}
238263
}
239264

240265
/** Helper class for gRPC calls that can return paginated results. */
Original file line numberDiff line numberDiff line change
@@ -159,18 +159,18 @@ public void run() {
159159

160160
@Test
161161
public void closeQuicklyDoesNotBlockIndefinitely() throws Exception {
162-
pool.closeAsync().get();
162+
pool.closeAsync(new SpannerImpl.ClosedException()).get();
163163
}
164164

165165
@Test
166166
public void closeAfterInitialCreateDoesNotBlockIndefinitely() throws Exception {
167167
pool.getReadSession().close();
168-
pool.closeAsync().get();
168+
pool.closeAsync(new SpannerImpl.ClosedException()).get();
169169
}
170170

171171
@Test
172172
public void closeWhenSessionsActiveFinishes() throws Exception {
173173
Session session = pool.getReadSession();
174-
pool.closeAsync().get();
174+
pool.closeAsync(new SpannerImpl.ClosedException()).get();
175175
}
176176
}
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ public void run() {
322322
assertThat(maxAliveSessions).isAtMost(maxSessions);
323323
}
324324
stopMaintenance.set(true);
325-
pool.closeAsync().get();
325+
pool.closeAsync(new SpannerImpl.ClosedException()).get();
326326
Exception e = getFailedError();
327327
if (e != null) {
328328
throw e;
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import com.google.cloud.spanner.SessionPool.Clock;
4343
import com.google.cloud.spanner.SessionPool.PooledSession;
4444
import com.google.cloud.spanner.SessionPool.SessionConsumerImpl;
45+
import com.google.cloud.spanner.SpannerImpl.ClosedException;
4546
import com.google.cloud.spanner.TransactionRunner.TransactionCallable;
4647
import com.google.cloud.spanner.TransactionRunnerImpl.TransactionContextImpl;
4748
import com.google.cloud.spanner.spi.v1.SpannerRpc;
@@ -58,6 +59,8 @@
5859
import com.google.spanner.v1.RollbackRequest;
5960
import io.opencensus.metrics.LabelValue;
6061
import io.opencensus.metrics.MetricRegistry;
62+
import java.io.PrintWriter;
63+
import java.io.StringWriter;
6164
import java.util.ArrayList;
6265
import java.util.Arrays;
6366
import java.util.Collection;
@@ -165,6 +168,26 @@ public void run() {
165168
Mockito.anyInt(), Mockito.anyBoolean(), any(SessionConsumer.class));
166169
}
167170

171+
@Test
172+
public void testClosedPoolIncludesClosedException() {
173+
pool = createPool();
174+
assertThat(pool.isValid()).isTrue();
175+
closePoolWithStacktrace();
176+
try {
177+
pool.getReadSession();
178+
fail("missing expected exception");
179+
} catch (IllegalStateException e) {
180+
assertThat(e.getCause()).isInstanceOf(ClosedException.class);
181+
StringWriter sw = new StringWriter();
182+
e.getCause().printStackTrace(new PrintWriter(sw));
183+
assertThat(sw.toString()).contains("closePoolWithStacktrace");
184+
}
185+
}
186+
187+
private void closePoolWithStacktrace() {
188+
pool.closeAsync(new SpannerImpl.ClosedException());
189+
}
190+
168191
@Test
169192
public void sessionCreation() {
170193
setupMockSessionCreation();
@@ -203,7 +226,7 @@ public void poolLifo() {
203226
public void poolClosure() throws Exception {
204227
setupMockSessionCreation();
205228
pool = createPool();
206-
pool.closeAsync().get(5L, TimeUnit.SECONDS);
229+
pool.closeAsync(new SpannerImpl.ClosedException()).get(5L, TimeUnit.SECONDS);
207230
}
208231

209232
@Test
@@ -237,7 +260,7 @@ public void run() {
237260
// Clear the exception to suppress logging of expected exceptions.
238261
Session.clearException();
239262
session1.close();
240-
pool.closeAsync().get(5L, TimeUnit.SECONDS);
263+
pool.closeAsync(new SpannerImpl.ClosedException()).get(5L, TimeUnit.SECONDS);
241264
verify(mockSession1).asyncClose();
242265
verify(mockSession2).asyncClose();
243266
}
@@ -260,7 +283,7 @@ public void run() {
260283
}
261284
})
262285
.start();
263-
pool.closeAsync().get(5L, TimeUnit.SECONDS);
286+
pool.closeAsync(new SpannerImpl.ClosedException()).get(5L, TimeUnit.SECONDS);
264287
stop.set(true);
265288
}
266289

@@ -316,7 +339,7 @@ public Void call() throws Exception {
316339
CountDownLatch latch = new CountDownLatch(1);
317340
getSessionAsync(latch, failed);
318341
insideCreation.await();
319-
pool.closeAsync();
342+
pool.closeAsync(new SpannerImpl.ClosedException());
320343
releaseCreation.countDown();
321344
latch.await();
322345
assertThat(failed.get()).isTrue();
@@ -374,7 +397,7 @@ public Void call() throws Exception {
374397
CountDownLatch latch = new CountDownLatch(1);
375398
getReadWriteSessionAsync(latch, failed);
376399
insideCreation.await();
377-
pool.closeAsync();
400+
pool.closeAsync(new SpannerImpl.ClosedException());
378401
releaseCreation.countDown();
379402
latch.await();
380403
assertThat(failed.get()).isTrue();
@@ -411,7 +434,7 @@ public Void call() throws Exception {
411434
CountDownLatch latch = new CountDownLatch(1);
412435
getSessionAsync(latch, failed);
413436
insideCreation.await();
414-
ListenableFuture<Void> f = pool.closeAsync();
437+
ListenableFuture<Void> f = pool.closeAsync(new SpannerImpl.ClosedException());
415438
releaseCreation.countDown();
416439
f.get();
417440
assertThat(f.isDone()).isTrue();
@@ -456,7 +479,7 @@ public Session answer(InvocationOnMock invocation) throws Throwable {
456479
CountDownLatch latch = new CountDownLatch(1);
457480
getReadWriteSessionAsync(latch, failed);
458481
insidePrepare.await();
459-
ListenableFuture<Void> f = pool.closeAsync();
482+
ListenableFuture<Void> f = pool.closeAsync(new SpannerImpl.ClosedException());
460483
releasePrepare.countDown();
461484
f.get();
462485
assertThat(f.isDone()).isTrue();
@@ -487,7 +510,7 @@ public void run() {
487510
PooledSession Session = pool.getReadSession();
488511
// Suppress expected Session warning.
489512
Session.clearException();
490-
pool.closeAsync();
513+
pool.closeAsync(new SpannerImpl.ClosedException());
491514
expectedException.expect(IllegalStateException.class);
492515
pool.getReadSession();
493516
}
@@ -925,7 +948,7 @@ public void run() {
925948
runMaintainanceLoop(clock, pool, cycles);
926949
// We will still close 2 sessions since at any point in time only 1 session was in use.
927950
assertThat(pool.numIdleSessionsRemoved()).isEqualTo(2L);
928-
pool.closeAsync().get(5L, TimeUnit.SECONDS);
951+
pool.closeAsync(new SpannerImpl.ClosedException()).get(5L, TimeUnit.SECONDS);
929952
}
930953

931954
@Test
@@ -976,7 +999,7 @@ public void run() {
976999
// The session pool only keeps MinSessions + MaxIdleSessions alive.
9771000
verify(session, times(options.getMinSessions() + options.getMaxIdleSessions()))
9781001
.singleUse(any(TimestampBound.class));
979-
pool.closeAsync().get(5L, TimeUnit.SECONDS);
1002+
pool.closeAsync(new SpannerImpl.ClosedException()).get(5L, TimeUnit.SECONDS);
9801003
}
9811004

9821005
@Test
@@ -1061,7 +1084,7 @@ public void run() {
10611084
assertThat(pool.getNumberOfAvailableWritePreparedSessions())
10621085
.isEqualTo((int) Math.ceil(options.getMinSessions() * options.getWriteSessionsFraction()));
10631086

1064-
pool.closeAsync().get(5L, TimeUnit.SECONDS);
1087+
pool.closeAsync(new SpannerImpl.ClosedException()).get(5L, TimeUnit.SECONDS);
10651088
}
10661089

10671090
private void waitForExpectedSessionPool(int expectedSessions, float writeFraction)
@@ -1447,7 +1470,7 @@ public Integer run(TransactionContext transaction) throws Exception {
14471470
.isTrue();
14481471
}
14491472
}
1450-
pool.closeAsync();
1473+
pool.closeAsync(new SpannerImpl.ClosedException());
14511474
}
14521475
}
14531476
}
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,11 @@
2727
import com.google.cloud.NoCredentials;
2828
import com.google.cloud.ServiceRpc;
2929
import com.google.cloud.grpc.GrpcTransportOptions;
30+
import com.google.cloud.spanner.SpannerImpl.ClosedException;
3031
import com.google.cloud.spanner.spi.v1.SpannerRpc;
3132
import com.google.spanner.v1.ExecuteSqlRequest.QueryOptions;
33+
import java.io.PrintWriter;
34+
import java.io.StringWriter;
3235
import java.util.Collections;
3336
import java.util.HashMap;
3437
import java.util.Map;
@@ -222,13 +225,36 @@ public void testClientId() {
222225

223226
// Get a database client for the same database as the first database. As this goes through a
224227
// different Spanner instance with potentially different options, it will get a different
225-
// client
226-
// id.
228+
// client id.
227229
DatabaseClientImpl databaseClient3 = (DatabaseClientImpl) spanner.getDatabaseClient(db);
228230
assertThat(databaseClient3.clientId).isEqualTo("client-2");
229231
}
230232
}
231233

234+
@Test
235+
public void testClosedException() {
236+
Spanner spanner = new SpannerImpl(rpc, spannerOptions);
237+
assertThat(spanner.isClosed()).isFalse();
238+
// Close the Spanner instance in a different method so we can actually verify that the entire
239+
// stacktrace of the method that closed the instance is included in the exception that will be
240+
// thrown by the instance after it has been closed.
241+
closeSpannerAndIncludeStacktrace(spanner);
242+
assertThat(spanner.isClosed()).isTrue();
243+
try {
244+
spanner.getDatabaseClient(DatabaseId.of("p", "i", "d"));
245+
fail("missing expected exception");
246+
} catch (IllegalStateException e) {
247+
assertThat(e.getCause()).isInstanceOf(ClosedException.class);
248+
StringWriter sw = new StringWriter();
249+
e.getCause().printStackTrace(new PrintWriter(sw));
250+
assertThat(sw.toString()).contains("closeSpannerAndIncludeStacktrace");
251+
}
252+
}
253+
254+
private void closeSpannerAndIncludeStacktrace(Spanner spanner) {
255+
spanner.close();
256+
}
257+
232258
private SpannerOptions createSpannerOptions() {
233259
return SpannerOptions.newBuilder()
234260
.setProjectId("[PROJECT]")

0 commit comments

Comments
 (0)