Skip to content

Commit a343ed6

Browse files
fix: serialize open*/close so racing opens cannot create orphan pools
openTableAsync / openAuthorizedViewAsync / openMaterializedViewAsync read 'closed' lock-free, then constructed the pool, then inserted it into sessionPools — close() could CAS closed=true and snapshot sessionPools in between, leaving the new pool orphaned: never closed, its callbacks landing on shut-down executors. Hold sessionPools' monitor across the closed check, the construction, and the insert. Opens are infrequent (typically once per table at app startup) so the monitor cost is negligible. Move close()'s closed flip inside the same monitor too. With every access now under the lock, downgrade 'closed' from AtomicBoolean to a plain boolean — the CAS provided no value over a plain read+write under the lock.
1 parent 42394fa commit a343ed6

1 file changed

Lines changed: 70 additions & 60 deletions

File tree

  • java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/api

java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/api/Client.java

Lines changed: 70 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -104,10 +104,10 @@ public class Client implements AutoCloseable {
104104
private final Resource<ClientConfigurationManager> configManager;
105105

106106
private final Set<SessionPool<?>> sessionPools = Collections.newSetFromMap(new WeakHashMap<>());
107-
// Set true at the start of close(); guards openTableAsync / openAuthorizedViewAsync /
108-
// openMaterializedViewAsync so concurrent opens during shutdown don't create pools the close
109-
// path won't see.
110-
private final AtomicBoolean closed = new AtomicBoolean(false);
107+
// Guarded by sessionPools' monitor: close() sets it before snapshotting the pool set, and the
108+
// open* methods check it before adding a new pool, so a racing open cannot insert a pool that
109+
// close() has already missed in its snapshot.
110+
private boolean closed = false;
111111

112112
public static Client create(ClientSettings settings) throws IOException {
113113
FeatureFlags featureFlags =
@@ -235,12 +235,12 @@ public Client(
235235

236236
@Override
237237
public void close() {
238-
if (!closed.compareAndSet(false, true)) {
239-
return; // idempotent
240-
}
241-
242238
List<SessionPool<?>> toClose;
243239
synchronized (sessionPools) {
240+
if (closed) {
241+
return; // idempotent
242+
}
243+
closed = true;
244244
toClose = new ArrayList<>(sessionPools);
245245
}
246246

@@ -292,67 +292,77 @@ public void close() {
292292
backgroundExecutor.close();
293293
}
294294

295-
private void checkNotClosed() {
296-
if (closed.get()) {
297-
throw new IllegalStateException("Client is closed");
298-
}
299-
}
300-
295+
// The closed check and pool insertion run under sessionPools' monitor so close() (which flips
296+
// closed under the same monitor) cannot snapshot the pool set between our check and our insert.
297+
// Opens are infrequent (typically once per table at app startup), so holding the monitor across
298+
// createAndStart is acceptable.
301299
public TableAsync openTableAsync(String tableId, Permission permission) {
302-
checkNotClosed();
303-
TableAsync tableAsync =
304-
TableAsync.createAndStart(
305-
featureFlags,
306-
clientInfo,
307-
configManager.get(),
308-
channelPool,
309-
defaultCallOptions,
310-
tableId,
311-
permission,
312-
metrics.get(),
313-
sessionTimer,
314-
userCallbackExecutor.get());
315-
sessionPools.add(tableAsync.getSessionPool());
316-
return tableAsync;
300+
synchronized (sessionPools) {
301+
if (closed) {
302+
throw new IllegalStateException("Client is closed");
303+
}
304+
TableAsync tableAsync =
305+
TableAsync.createAndStart(
306+
featureFlags,
307+
clientInfo,
308+
configManager.get(),
309+
channelPool,
310+
defaultCallOptions,
311+
tableId,
312+
permission,
313+
metrics.get(),
314+
sessionTimer,
315+
userCallbackExecutor.get());
316+
sessionPools.add(tableAsync.getSessionPool());
317+
return tableAsync;
318+
}
317319
}
318320

319321
public AuthorizedViewAsync openAuthorizedViewAsync(
320322
String tableId, String viewId, OpenAuthorizedViewRequest.Permission permission) {
321-
checkNotClosed();
322-
AuthorizedViewAsync viewAsync =
323-
AuthorizedViewAsync.createAndStart(
324-
featureFlags,
325-
clientInfo,
326-
configManager.get(),
327-
channelPool,
328-
defaultCallOptions,
329-
tableId,
330-
viewId,
331-
permission,
332-
metrics.get(),
333-
sessionTimer,
334-
userCallbackExecutor.get());
335-
sessionPools.add(viewAsync.getSessionPool());
336-
return viewAsync;
323+
synchronized (sessionPools) {
324+
if (closed) {
325+
throw new IllegalStateException("Client is closed");
326+
}
327+
AuthorizedViewAsync viewAsync =
328+
AuthorizedViewAsync.createAndStart(
329+
featureFlags,
330+
clientInfo,
331+
configManager.get(),
332+
channelPool,
333+
defaultCallOptions,
334+
tableId,
335+
viewId,
336+
permission,
337+
metrics.get(),
338+
sessionTimer,
339+
userCallbackExecutor.get());
340+
sessionPools.add(viewAsync.getSessionPool());
341+
return viewAsync;
342+
}
337343
}
338344

339345
public MaterializedViewAsync openMaterializedViewAsync(
340346
String viewId, OpenMaterializedViewRequest.Permission permission) {
341-
checkNotClosed();
342-
MaterializedViewAsync viewAsync =
343-
MaterializedViewAsync.createAndStart(
344-
featureFlags,
345-
clientInfo,
346-
configManager.get(),
347-
channelPool,
348-
defaultCallOptions,
349-
viewId,
350-
permission,
351-
metrics.get(),
352-
sessionTimer,
353-
userCallbackExecutor.get());
354-
sessionPools.add(viewAsync.getSessionPool());
355-
return viewAsync;
347+
synchronized (sessionPools) {
348+
if (closed) {
349+
throw new IllegalStateException("Client is closed");
350+
}
351+
MaterializedViewAsync viewAsync =
352+
MaterializedViewAsync.createAndStart(
353+
featureFlags,
354+
clientInfo,
355+
configManager.get(),
356+
channelPool,
357+
defaultCallOptions,
358+
viewId,
359+
permission,
360+
metrics.get(),
361+
sessionTimer,
362+
userCallbackExecutor.get());
363+
sessionPools.add(viewAsync.getSessionPool());
364+
return viewAsync;
365+
}
356366
}
357367

358368
public static class Resource<T> {

0 commit comments

Comments
 (0)