From 6e4343d1e3aafc455c7a834e02333ad6979dd85d Mon Sep 17 00:00:00 2001 From: Rahul Yadav Date: Fri, 10 Jul 2026 23:51:52 +0530 Subject: [PATCH] fix(spanner): avoid data race on DIRECTPATH_CHANNEL_CREATED by using volatile GapicSpannerRpc declared a process-wide mutable static flag 'public static boolean DIRECTPATH_CHANNEL_CREATED' that is written from the GapicSpannerRpc constructor without synchronization and read by HeaderInterceptor when populating the directpath_enabled built-in metrics attribute. Constructing multiple Spanner clients concurrently (for example Apache Beam DirectRunner initializing DoFns on multiple worker threads) makes that write/read pair a data race, which ThreadSanitizer reports and halts the JVM on. Declare the field volatile so the constructor write happens-before every subsequent read. The flag has no read-modify-write usage, so volatile is sufficient, and it preserves the field's boolean source and binary signature as well as the existing last-writer-wins behavior. Document the concurrency semantics on the field. Add a regression test that constructs eight GapicSpannerRpc instances concurrently against the in-process mock server, verifies construction succeeds with the flag in a consistent state, and asserts via reflection that the field stays volatile. --- .../cloud/spanner/spi/v1/GapicSpannerRpc.java | 10 +++- .../spanner/spi/v1/GapicSpannerRpcTest.java | 49 +++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java b/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java index 831e5bf69da0..49b1cca60c43 100644 --- a/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java +++ b/java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java @@ -255,7 +255,15 @@ public class GapicSpannerRpc implements SpannerRpc { private static final String CLIENT_LIBRARY_LANGUAGE = "spanner-java"; public static final String DEFAULT_USER_AGENT = CLIENT_LIBRARY_LANGUAGE + "/" + GaxProperties.getLibraryVersion(GapicSpannerRpc.class); - public static boolean DIRECTPATH_CHANNEL_CREATED = false; + + /** + * Whether the most recently initialized RPC created a DirectPath channel. + * + *

This process-wide volatile value may be updated during concurrent client construction and + * read from another thread when built-in metric attributes are created. + */ + public static volatile boolean DIRECTPATH_CHANNEL_CREATED = false; + private static final String API_FILE = "grpc-gcp-apiconfig.json"; private final RequestIdCreator requestIdCreator = new RequestIdCreatorImpl(); diff --git a/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java b/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java index e39d6be2a33c..275bbe66c20d 100644 --- a/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java +++ b/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java @@ -106,14 +106,20 @@ import java.lang.reflect.Modifier; import java.net.InetSocketAddress; import java.time.Duration; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.IdentityHashMap; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -934,6 +940,49 @@ public void testAdminStubSettings_whenStubNotInitialized_assertNullClientSetting rpc.shutdown(); } + @Test + public void testConcurrentClientCreationDoesNotRaceOnDirectPathFlag() throws Exception { + // Concurrent creation of Spanner clients used to cause a data race on the static + // DIRECTPATH_CHANNEL_CREATED field, which was written from the constructor without + // synchronization. This verifies that concurrent client creation succeeds and leaves the flag + // in a consistent state now that the field is volatile. + int numThreads = 8; + ExecutorService executor = Executors.newFixedThreadPool(numThreads); + CountDownLatch start = new CountDownLatch(1); + List> futures = new ArrayList<>(numThreads); + try { + for (int i = 0; i < numThreads; i++) { + futures.add( + executor.submit( + () -> { + start.await(); + GapicSpannerRpc rpc = new GapicSpannerRpc(createSpannerOptions(), true); + try { + return null; + } finally { + rpc.shutdown(); + } + })); + } + start.countDown(); + for (Future future : futures) { + future.get(60L, TimeUnit.SECONDS); + } + // The test options connect to a local plaintext mock server, so no DirectPath channel is + // ever created. + assertTrue( + Modifier.isVolatile( + GapicSpannerRpc.class.getField("DIRECTPATH_CHANNEL_CREATED").getModifiers())); + assertFalse(GapicSpannerRpc.DIRECTPATH_CHANNEL_CREATED); + } finally { + start.countDown(); + for (Future future : futures) { + future.cancel(true); + } + executor.shutdownNow(); + } + } + @Test public void testCreateSession_assertSessionProto() { SpannerOptions options = createSpannerOptions();