fix(spanner): avoid data race on DIRECTPATH_CHANNEL_CREATED by using volatile#13727
Open
rahul2393 wants to merge 1 commit into
Open
fix(spanner): avoid data race on DIRECTPATH_CHANNEL_CREATED by using volatile#13727rahul2393 wants to merge 1 commit into
rahul2393 wants to merge 1 commit into
Conversation
…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.
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses a potential data race during concurrent client construction by marking the static field DIRECTPATH_CHANNEL_CREATED as volatile in GapicSpannerRpc, and introduces a concurrent unit test to verify this fix. The review feedback recommends improving the test's cleanup block by adding a null check and awaiting the termination of the ExecutorService to prevent potential thread leaks and NullPointerExceptions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes b/509632639.
Problem
GapicSpannerRpcdeclared a process-wide mutable static flag:The flag is written from the
GapicSpannerRpcconstructor without synchronization and read byHeaderInterceptorwhen populating thedirectpath_enabledbuilt-in metrics attribute. When multiple Spanner clients are constructed concurrently (for example Apache Beam's DirectRunner initializingDoFns on multiple worker threads), the unsynchronized write/read pair is a data race; ThreadSanitizer halts the JVM on it.Fix
Declare the field
volatile:HeaderInterceptor, eliminating the data race. The flag has no read-modify-write usage, sovolatileis sufficient; the existing last-writer-wins semantics are preserved unchanged.volatilewas chosen overAtomicBooleandeliberately: it keeps the field's source and binary signature (boolean) intact, so any external code readingGapicSpannerRpc.DIRECTPATH_CHANNEL_CREATEDkeeps compiling and linking. (The class is@InternalApiand thespi.v1package is excluded from clirr checks, but there is no reason to break the field's ABI whenvolatilefixes the race equally well.)