Repository navigation
fix(storage): honor timeout in upload_chunks_concurrently - #18765
Open
abhishekacharya200 wants to merge 2 commits into
Open
abhishekacharya200 wants to merge 2 commits into
abhishekacharya200 wants to merge 2 commits into
Conversation
upload_chunks_concurrently accepted a timeout but never passed it to the XML multipart requests, so initiate, every part upload, finalize and cancel always ran with the (61, 60) default. Pass it through to all of them, including the _upload_part worker. Callers who don't pass a timeout now get the documented 60 seconds (_DEFAULT_TIMEOUT, as every Blob upload and download already sends) instead of (61, 60), so only the connect timeout moves by one second. Fixes googleapis#18764
Contributor
There was a problem hiding this comment.
Code Review
This pull request propagates the timeout parameter to various container and part operations within upload_chunks_concurrently (such as initiate, finalize, cancel, and _upload_part) and adds corresponding unit tests. The review feedback recommends wrapping the container.cancel call inside the exception handler in a nested try...except block to prevent cancellation failures from masking the original exception.
Contributor
|
/gcbrun (ccc12b7) |
shradhakatyal
requested changes
Oct 26, 2026
A failure in the cleanup cancel is now logged with the upload id, so it can no longer replace the exception that caused the cleanup.
Contributor
|
/gcbrun (1520e8b) |
shradhakatyal
self-requested a review
November 3, 2026 16:32
shradhakatyal
approved these changes
Nov 3, 2026
This branch has not been deployed
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 #18764
upload_chunks_concurrentlytakes atimeoutargument and documents it as the time to wait for the server response, but it never passed it on. The XML multipart requests it makes (initiate, each part upload, finalize, and cancel when something fails) all ran with the(61, 60)default of the media layer, so a caller could not make a stuck upload fail sooner or give a slow connection more time.This passes
timeouttoXMLMPUContainer.initiate,finalizeandcancel, and adds it to the_upload_partworker so everyXMLMPUPart.uploadgets it as well. Thread and process workers behave the same, since the timeout is a plain number or tuple.One small change for callers who don't pass a timeout: these requests now use the documented default of 60 seconds (
_DEFAULT_TIMEOUT, the value everyBlobupload and download already sends) instead of(61, 60). Only the connect timeout moves, by one second.The cleanup
cancelgets the same timeout as the other requests. If the cancel itself fails, for example with a very short timeout, the failure is logged as a warning with the upload id and the original upload error is still the one raised.test_upload_chunks_concurrently_keeps_original_error_when_cancel_failscovers that path.With the script from the issue, which records the timeout each request gets:
Added
test_upload_chunks_concurrently_passes_timeoutandtest_upload_chunks_concurrently_passes_timeout_to_cancel, which fail onmainand pass with this change. The existing tests that checked these calls now also check that the default timeout is sent, andtest__upload_partchecks that the worker hands its timeout to the part upload. The fulltests/unitsuite passes locally (2068 passed), and the noxfile'sruff==0.14.14import and format checks andflake8report nothing.