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

add timeouts to all requests #5881

Merged
merged 3 commits into from
Jun 19, 2022

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Jun 19, 2022

https://requests.readthedocs.io/en/latest/user/advanced/#timeouts

Most requests to external servers should have a timeout attached, in case the server is not responding in a timely manner. By default, requests do not time out unless a timeout value is set explicitly. Without a timeout, your code may hang for minutes or more.

Seems like not a great default.

I've added a timeout to all the requests that I could find. Probably fixes some of the examples of poetry being "slow" or "stuck" that have been reported eg #3352, though it's hard to be sure.

@mkniewallner
Copy link
Member

Should we make this configurable, like pip does for instance?
An arbitrary timeout may negatively impact users with unstable connections (especially on mobile networks, or on the move).

I would also suggest to define the default timeout value in a constant to avoid hard coding the value everywhere.

@dimbleby
Copy link
Contributor Author

dimbleby commented Jun 19, 2022

I'm probably against making this configurable, just another confusing thing for users to worry about.

Not sure I can be bothered about making all these value use a constant either. I don't know that there's any particularly compelling reason to think that we'd always want them all to have the same value anyway. Hmm, maybe.

I could see a case for picking a bigger number than 5 though. 5 seconds of inactivity is certainly a slow server, but perhaps not slow enough to completely give up on. Anyone want to propose a number? Perhaps pip's default of 15 is a sensible lead to follow.

@neersighted
Copy link
Member

I'm probably against making this configurable, just another confusing thing for users to worry about.

Not sure I can be bothered about making all these value use a constant either. I don't know that there's any particularly compelling reason to think that we'd always want them all to have the same value anyway. Hmm, maybe.

I could see a case for picking a bigger number than 5 though. 5 seconds of inactivity is certainly a slow server, but perhaps not slow enough to completely give up on. Anyone want to propose a number? Perhaps pip's default of 15 is a sensible lead to follow.

I would not be opposed to configuring this through an environment variable, though given that our existing config machinery reads from poetry.toml and environment variables it might be less confusing to allow for both. It's definitely not a knob I'm eager to add, but I do see it having practical use in some scenarios (hence an environment variable as a 'hidden' knob).

@dimbleby
Copy link
Contributor Author

I'll make it a constant and if y'all are motivated to make that configurable later then at least it'll only be one place to change.

Also make it 15s rather than 5s (matching the default in pip)
@mkniewallner
Copy link
Member

I'm probably against making this configurable, just another confusing thing for users to worry about.

I don't think this would confuse users, since the idea would be that most users won't even need to worry about configuring the timeout (since we have a sane default), but they would still have a knob to turn if they ever need to.

Case in point, I almost never use the --timeout option from pip, but it happened that I needed to use it while working on a train, with an ever-changing mobile connection through an hotspot and great chances of having timeouts, so I was glad to have the option when I needed to (even if most of the time, I don't have to worry about it).

@dimbleby
Copy link
Contributor Author

I won't be adding code to make this configurable.

Feel free to do as you see fit with this MR: merge it and add to it, or merge it and leave it alone, or reject it.

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants