-
Notifications
You must be signed in to change notification settings - Fork 151
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
Storage: Default timeout for requests breaks chunked downloads #18
Comments
@crwilcox Should we expose transport_kwargs for |
FWIW, there is a resumable media PR that I coincidentally opened yesterday which adds customizable timeouts. It's for uploads only, though, but the same changes can be added to downloads as well. A generalization of this would be exposing all |
@HemangChothani @crwilcox If there is a clear approach, I'm happy to support the implementation. Any thoughts on this? |
@bjoernpollex-sc Is the error you are seeing a Timeout error raised by the If so, there is a pull request awaiting review that will address this. The default timeout will no longer be interpreted as the "total timeout", but rather just a timeout for the transport layer for each request (as implemented by the |
@plamut Yes, the timeout is raised by that context manager. It sounds like your PR might fix my issue as well, thank you very much! I am not entirely sure, but my guess is that the timeout in my case gets triggered by context switched between threads, since the timeout is measured against the wall-clock, and the timed operations are not atomic. I can try it out sometime this week. |
@bjoernpollex-sc The yesterday's Until a new One note - the release also changed the default transport timeout to 120 seconds to prevent requests from hanging indefinitely. It's the same timeout that is already used for automatically refreshing credentials, but nevertheless mentioning this if it plays a role in your setup with all the threads, etc. |
@plamut Yeah, I already got notified, thanks! I'll try out the new release on Monday, and report here if it resolves the issue. |
@plamut I updated to |
@bjoernpollex-sc That's great to hear! I will still keep the issue open for a little while, though, because the PR that bumps the |
The default timeout introduced in googleapis/google-resumable-media-python#88 is causing crashes in our application. We are using chunked downloads by setting
chunk_size
on the blob, and then callingdownload_to_file
. Our application is multi-threaded, and we are actually downloading files into a custom stream that is backed by a ring-buffer (so writes may block until space is available again). In some cases, and I haven't figured out the pattern yet, our application hits the default timeout when fetching a new chunk of data insideAuthorizedSession.request
. Currently,google.cloud.storage
offers no way to use a custom transport (as suggested here), so this workaround is not applicable, and there is no way to override the timeout.I can't provide a simple reproduction here yet, I'm still investigating, but since it's an issues that only occurs sporadically, I'm not even sure that's possible. I'm wondering if maybe in some scenarios Python's multi-threading just hits an unlucky timing, and the thread running the request doesn't get scheduled for a longer time than usual, causing the observed timeout to be much higher. Not sure how to test this though.
I'm posting here, because the upstream change was made to fix googleapis/google-cloud-python#5909, I'm not sure what the proper fix would be.
The text was updated successfully, but these errors were encountered: