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();