Skip to content

Commit 1e05ea5

Browse files
authored
fix(spanner): avoid data race on DIRECTPATH_CHANNEL_CREATED by using volatile (#13727)
Fixes b/509632639. ## Problem `GapicSpannerRpc` declared a process-wide mutable static flag: ```java public static boolean DIRECTPATH_CHANNEL_CREATED = false; ``` The flag is written from the `GapicSpannerRpc` constructor without synchronization and read by `HeaderInterceptor` when populating the `directpath_enabled` built-in metrics attribute. When multiple Spanner clients are constructed concurrently (for example Apache Beam's DirectRunner initializing `DoFn`s on multiple worker threads), the unsynchronized write/read pair is a data race; ThreadSanitizer halts the JVM on it. ## Fix Declare the field `volatile`: ```java public static volatile boolean DIRECTPATH_CHANNEL_CREATED = false; ``` - The volatile write in the constructor happens-before every subsequent volatile read in `HeaderInterceptor`, eliminating the data race. The flag has no read-modify-write usage, so `volatile` is sufficient; the existing last-writer-wins semantics are preserved unchanged. - `volatile` was chosen over `AtomicBoolean` deliberately: it keeps the field's source and binary signature (`boolean`) intact, so any external code reading `GapicSpannerRpc.DIRECTPATH_CHANNEL_CREATED` keeps compiling and linking. (The class is `@InternalApi` and the `spi.v1` package is excluded from clirr checks, but there is no reason to break the field's ABI when `volatile` fixes the race equally well.) - Javadoc on the field documents the concurrency semantics.
1 parent 4fb3f4b commit 1e05ea5

2 files changed

Lines changed: 58 additions & 1 deletion

File tree

java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,15 @@ public class GapicSpannerRpc implements SpannerRpc {
255255
private static final String CLIENT_LIBRARY_LANGUAGE = "spanner-java";
256256
public static final String DEFAULT_USER_AGENT =
257257
CLIENT_LIBRARY_LANGUAGE + "/" + GaxProperties.getLibraryVersion(GapicSpannerRpc.class);
258-
public static boolean DIRECTPATH_CHANNEL_CREATED = false;
258+
259+
/**
260+
* Whether the most recently initialized RPC created a DirectPath channel.
261+
*
262+
* <p>This process-wide volatile value may be updated during concurrent client construction and
263+
* read from another thread when built-in metric attributes are created.
264+
*/
265+
public static volatile boolean DIRECTPATH_CHANNEL_CREATED = false;
266+
259267
private static final String API_FILE = "grpc-gcp-apiconfig.json";
260268

261269
private final RequestIdCreator requestIdCreator = new RequestIdCreatorImpl();

java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,14 +106,20 @@
106106
import java.lang.reflect.Modifier;
107107
import java.net.InetSocketAddress;
108108
import java.time.Duration;
109+
import java.util.ArrayList;
109110
import java.util.Collection;
110111
import java.util.Collections;
111112
import java.util.HashMap;
112113
import java.util.IdentityHashMap;
114+
import java.util.List;
113115
import java.util.Map;
114116
import java.util.Objects;
115117
import java.util.Set;
118+
import java.util.concurrent.CountDownLatch;
116119
import java.util.concurrent.Executor;
120+
import java.util.concurrent.ExecutorService;
121+
import java.util.concurrent.Executors;
122+
import java.util.concurrent.Future;
117123
import java.util.concurrent.ScheduledExecutorService;
118124
import java.util.concurrent.TimeUnit;
119125
import java.util.concurrent.atomic.AtomicBoolean;
@@ -934,6 +940,49 @@ public void testAdminStubSettings_whenStubNotInitialized_assertNullClientSetting
934940
rpc.shutdown();
935941
}
936942

943+
@Test
944+
public void testConcurrentClientCreationDoesNotRaceOnDirectPathFlag() throws Exception {
945+
// Concurrent creation of Spanner clients used to cause a data race on the static
946+
// DIRECTPATH_CHANNEL_CREATED field, which was written from the constructor without
947+
// synchronization. This verifies that concurrent client creation succeeds and leaves the flag
948+
// in a consistent state now that the field is volatile.
949+
int numThreads = 8;
950+
ExecutorService executor = Executors.newFixedThreadPool(numThreads);
951+
CountDownLatch start = new CountDownLatch(1);
952+
List<Future<Void>> futures = new ArrayList<>(numThreads);
953+
try {
954+
for (int i = 0; i < numThreads; i++) {
955+
futures.add(
956+
executor.submit(
957+
() -> {
958+
start.await();
959+
GapicSpannerRpc rpc = new GapicSpannerRpc(createSpannerOptions(), true);
960+
try {
961+
return null;
962+
} finally {
963+
rpc.shutdown();
964+
}
965+
}));
966+
}
967+
start.countDown();
968+
for (Future<Void> future : futures) {
969+
future.get(60L, TimeUnit.SECONDS);
970+
}
971+
// The test options connect to a local plaintext mock server, so no DirectPath channel is
972+
// ever created.
973+
assertTrue(
974+
Modifier.isVolatile(
975+
GapicSpannerRpc.class.getField("DIRECTPATH_CHANNEL_CREATED").getModifiers()));
976+
assertFalse(GapicSpannerRpc.DIRECTPATH_CHANNEL_CREATED);
977+
} finally {
978+
start.countDown();
979+
for (Future<Void> future : futures) {
980+
future.cancel(true);
981+
}
982+
executor.shutdownNow();
983+
}
984+
}
985+
937986
@Test
938987
public void testCreateSession_assertSessionProto() {
939988
SpannerOptions options = createSpannerOptions();

0 commit comments

Comments
 (0)