-
Notifications
You must be signed in to change notification settings - Fork 1k
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
30s default http read timeout #3182
Conversation
Since we're now using read timeouts and not total timeouts, we can use a lower threshold, a single read shouldn't take 5 min (and not even 10s).
I think we should probably use a larger value given all the problems we've seen with timeouts? |
I guess I'd still prefer something like 30s, agree that 5 minutes is execessive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ty
let default_timeout = 5 * 60; | ||
let default_timeout = 30; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this documented anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the 5 min were previously undocumented.
It's also a bit misleading since it's not an http timeout properly, it's just a read timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I don't think we'll ever want a real timeout per http request, I think a read timeout makes sense. If we expose a comprehensive timeout, I think it'd expect it to be like for an entire command invocation or installation of a package or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth,
Poetry and Pip use 15 seconds as a default
- https://python-poetry.org/docs/faq/#my-requests-are-timing-out
- https://github.com/pypa/pip/blob/main/src/pip/_internal/cli/cmdoptions.py#L294
I'll still take 30 seconds over 300 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really like to have some data on this what real world values are
Since we're now using read timeouts and not total timeouts, we can use a lower threshold, a single read shouldn't take 5 min (and not even 10s).
The 10s value is somewhat arbitrary.
Like #3144, this is a breaking change in some sense.