Skip to content

fix(gensupport): fix transferChunk race condition by returning response with non-cancelled context. #3258

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.

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

Merged
merged 12 commits into from
Aug 5, 2025

Conversation

meet2mky
Copy link
Contributor

@meet2mky meet2mky commented Aug 4, 2025

The previous implementation used context.WithTimeout to manage timeouts for each chunk upload and cancelled the upload when the response was received. This introduced a race condition when the response was not yet processed by caller and context was cancelled.

This fix removes the race condition by doing following things:

  • Starts a timer and the upload request in a separate goroutine.
  • Uses a select statement to wait for either the upload to complete, the timer to fire, or the context to be cancelled.
  • Explicitly cancels the upload request if the timer fires first, preventing the race condition by not cancelling the context when response is succeess (which waits for caller to process the body) and ensuring resources are cleaned up correctly.
  • Renamed two variable to make code more readable.
    Internal Bug: 435359905

@meet2mky meet2mky requested a review from a team as a code owner August 4, 2025 17:53
@meet2mky meet2mky changed the title fix(gensupport): fix transferChunk to return un-cancelled response which fixes race condition. fix(gensupport): fix transferChunk race condition by returning response with non-cancelled context. Aug 4, 2025
Copy link

@raj-prince raj-prince left a comment

Choose a reason for hiding this comment

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

LGTM overall, few nits.

@meet2mky meet2mky requested a review from raj-prince August 5, 2025 09:00
@codyoss codyoss enabled auto-merge (squash) August 5, 2025 17:42
@codyoss codyoss merged commit 091d422 into googleapis:main Aug 5, 2025
5 checks passed
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.

4 participants