Skip to content

fix: data race in ConnectionWorker.#12915

Open
agrawal-siddharth wants to merge 1 commit intogoogleapis:mainfrom
agrawal-siddharth:fix_race
Open

fix: data race in ConnectionWorker.#12915
agrawal-siddharth wants to merge 1 commit intogoogleapis:mainfrom
agrawal-siddharth:fix_race

Conversation

@agrawal-siddharth
Copy link
Copy Markdown
Contributor

RACE DESCRIPTION:

Requests are added to inflightRequestQueue before actually being sent. A race occurs when a response arrives on a separate thread, triggering a retry process that clears inflightRequestQueue and nullifies requestSendTimeStamp. Simultaneously, the appendLoop thread might try to send a request from the localQueue, setting requestSendTimeStamp to the current time, leading to conflicting writes.

FIX DESCRIPTION:

The fix is to ensure only the appendLoop thread modifies requestSendTimeStamp. This thread confinement prevents conflicts with other threads, like the response thread. We can stop nullifying requestSendTimeStamp because appendLoop will always update it to the current time just before sending. The check for exceeding response timeouts happens earlier in the appendLoop, so this change does not cause issues.

@agrawal-siddharth agrawal-siddharth requested review from a team as code owners April 24, 2026 03:32
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the management of requestSendTimeStamp within ConnectionWorker.java by removing explicit nullification and documenting that it is only modified by the appendLoop thread. A review comment identifies a potential visibility issue and recommends marking the field as volatile to ensure other threads can safely read the updated timestamp.

Comment on lines +1502 to 1503
// This is set ONLY by the appendLoop thread.
Instant requestSendTimeStamp;
Copy link
Copy Markdown
Contributor

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;

RACE DESCRIPTION:

Requests are added to inflightRequestQueue before actually being sent. A race occurs when a response arrives on a separate thread, triggering a retry process that clears inflightRequestQueue and nullifies requestSendTimeStamp. Simultaneously, the appendLoop thread might try to send a request from the localQueue, setting requestSendTimeStamp to the current time, leading to conflicting writes.

FIX DESCRIPTION:

The fix is to ensure only the appendLoop thread modifies requestSendTimeStamp. This thread confinement prevents conflicts with other threads, like the response thread. We can stop nullifying requestSendTimeStamp because appendLoop will always update it to the current time just before sending. The check for exceeding response timeouts happens earlier in the appendLoop, so this change does not cause issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant