Skip to content

fix: data race in ConnectionWorker. #12915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
agrawal-siddharth wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: data race in ConnectionWorker. #12915

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions ...-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,7 +878,6 @@ private void appendLoop() {
// Consider the backend latency as completed for the current request.
requestProfilerHook.endOperation(
RequestProfiler.OperationName.RESPONSE_LATENCY, requestWrapper.requestUniqueId);
requestWrapper.requestSendTimeStamp = null;
requestProfilerHook.startOperation(
RequestProfiler.OperationName.WAIT_QUEUE, requestWrapper.requestUniqueId);
waitingRequestQueue.addFirst(requestWrapper);
Expand Down Expand Up @@ -1454,7 +1453,6 @@ private void doneCallback(Throwable finalStatus) {
private AppendRequestAndResponse pollInflightRequestQueue(boolean pollLast) {
AppendRequestAndResponse requestWrapper =
pollLast ? inflightRequestQueue.pollLast() : inflightRequestQueue.poll();
requestWrapper.requestSendTimeStamp = null;
--this.inflightRequests;
this.inflightBytes -= requestWrapper.messageSize;
this.inflightReduced.signal();
Expand Down Expand Up @@ -1501,7 +1499,7 @@ static final class AppendRequestAndResponse {
long recordBatchRowCount = -1;

// Time at which request was last sent over the network.
// If a response is no longer expected this is set back to null.
// This is set ONLY by the appendLoop thread.
Instant requestSendTimeStamp;
Comment on lines +1502 to 1503
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Mark requestSendTimeStamp as volatile to ensure that updates from the appendLoop thread are visible to other threads reading the timestamp (e.g., for monitoring or timeout checks). While thread confinement prevents write-write races, volatile is necessary to prevent read-write visibility issues in a concurrent environment.

Suggested change
// This is set ONLY by the appendLoop thread.
Instant requestSendTimeStamp;
// This is set ONLY by the appendLoop thread.
volatile Instant requestSendTimeStamp;


AppendRequestAndResponse(
Expand Down
Loading