Skip to content
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

Make a clear distinction between the transport timeout and the total method time timeout #423

Closed
plamut opened this issue Jan 16, 2020 · 0 comments · Fixed by #424
Closed
Assignees
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@plamut
Copy link
Contributor

plamut commented Jan 16, 2020

It turned out that the concern from #406 was valid, unfortunately:

A downstream library that is timeout-sensitive might still be affected by the change, because the new explicit timeout parameter changes the semantics of AuthorizedSession.request() . The timeout represents the total allowed time for all requests made behind the scenes (e.g. credentials refresh attempts), as opposed to a previous per-request timeout.

The changed semantics of timeout caused an issue in the storage library. A user reported that if a large file upload takes, say, minutes and succeeds, a Timeout error is still raised, because the total execution time takes more than ~60 seconds, which the default transport timeout configured here in this library here.

Proposed solution

Leave the timeout parameter as is and still pass it to the transport, but introduce and expose a new optional parameter max_allowed_time that can be used instead to configure the "total timeout".

NOTE: The method itself might still take longer than max_allowed_time, because the underlying request might take longer, which is the nature of the requests library's timeout (docs).

Alternatives

Modifying the library code so that it supports "absolute total timeouts", but that would require interrupting requests in-flight, which does not seem to be a straightforward task -https://stackoverflow.com/q/21965484/5040035

@plamut plamut added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Jan 16, 2020
@plamut plamut self-assigned this Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant