-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Consider making Timeout option required or have a default #3070
Comments
This is an entirely reasonable suggestion. Honestly, I'm not averse to doing it: there are definitely worse things to do than this. I'd be open to providing a default timeout. However, I don't think we can do it until 3.0.0. Of course @kennethreitz as keeper of the spirit of requests has got the final say on this, and should definitely weigh in. |
Changing the default would definitely need to wait until 3.0.0. I'm not against having a well-reasoned (and therefore, also well-documented) default timeout parameter. I'm still pretty strongly against sessions having a timeout attribute though and I think that shouldn't muddy the waters of this discussion because it will sidetrack things and prevent an otherwise productive discussion. |
@sigmavirus24 I don't know the argument pro/con sessions having a timeout, but I'm happy to defer to other's judgement and experience on that, keeping these waters clean, like you say. I'm just happy that I haven't gotten a swift education about timeouts. |
I'm definitely in favor of this in light of dealing with a package that doesn't provide a way to set a timeout, but does accept a fully configured session. My current fix is a specialized class TimeoutHTTPAdapter(HTTPAdapter):
def __init__(self, timeout, *args, **kwargs):
self._timeout = timeout
super().__init__(*args, **kwargs)
def send(self, request, timeout=False, ...):
if timeout is None:
timeout = self._timeout
return super().send(request, timeout=timeout, ...) I'd much prefer something like: session = Session(timeout=...)
# or even
session.default_timeout = ... However, this current fix isn't too cumbersome either. |
👍 for |
@kuraga No. Per @sigmavirus24, and in many many previous discussions:
|
@Lukasa ok, but which discussion did you cite? Was it private? Which previous discussions? |
Argh, sorry, copy-paste fail:
|
@kuraga please search closed issues. There are several discussions of this. I really don't want this diversion to distract from the topic of a default timeout though. So can we please stop discussing this now as I asked nicely before. You and @justanr are distracting from the important and attainable portion of this issue. |
@sigmavirus24 I added a caveat to my initial comment, above. Hopefully that should get this discussion focused. |
I'm (as a user) slightly against this proposal.
What I've missed? |
At the very least, how about we include the |
It's extremely unclear to me what "wrong" means here. They aren't wrong: they work, as designed. The introductory examples are all clearly typed into the Python interactive interpreter, so they have a timeout: the user sitting there can hit ^C anytime they like. I'd welcome enhancements to the "timeouts" section of the documentation to be more emphatic, that certainly does seem reasonable. But I don't think we need to go through and add the |
I think the point that's generally acknowledged by this bug is that the design was wrong. There's a general consensus here that adding a default timeout or requiring it as an argument is a good idea. The docs could address that, and that seems like a simple step to take until this issue is resolved in the next major version. What happens in practice is that people grab the examples from the docs, don't read the timeout section carefully (or don't understand its implications -- I didn't for years until I got bit), and then wind up with programs that can hang forever. I completely agree with @chris-martin that until this issue is fixed, all examples in the docs should provide the timeout argument. Otherwise, we're providing examples that can (and probably will) break your programs. |
There is a substantial difference between "is a good idea in production code" and "should be used in every example". For example, all production code should use Sessions for everything, but the documentation doesn't do that because it doesn't help teach the specific lessons that the documentation is intended to teach. While I appreciate the concern that users will blindly copy code out of the documentation without further consideration, anyone doing that in their product is necessarily going to have sub-par results. This is true in all programming tools. |
That's fair enough, @Lukasa. What about making the Timeout section more explicit then? Right now it says:
And then goes on with a long, somewhat complicated warning. Could the first part say:
Something like that? |
Yup, I'd be happy to merge a PR with that change in it. 😄 |
Partially addresses psf#3070.
I think that we've addressed this through documentation. I don't think any of the core team is going to change the API to require a timeout and I think we've done our diligence here. |
I doubt this will change anybody's mind, but I vehemently disagree with closing this bug without a fix. We improved the documentation via the PR that I filed, but in practice I regularly run into programs that hang because they do not have a timeout. It's gotten bad enough that it's one of the first things I think of when a program hangs. Requests is a wonderful library but you can't document your way out of a problem like this. Look at the list of issues referencing this one. Documentation is just not enough, or else issues would stop referencing this one. I respect the requests maintainers greatly, but I hope you'll reconsider closing this. If it causes API changes, that's what version bumps are for. But I'm not even sure API changes would be needed. Please reconsider. |
We could consider making a default in 3.0, but I don't know what it would be... 120s? Idk, I like our current design. You really shouldn't be hitting the internet without proper timeouts in place ever in production. Any sane engineer knows that — it's not Requests' job to do your job for you. |
Here are a few example idle timeout defaults from other libraries, in case that helps:
|
This one got quite some negative thumbs, but I think it's a good idea. The requests library already relies on the environment, i.e. it honors a .netrc and it honors proxy settings - and it has a Such a solution may empower quite some end-users. Some of the problem with this issue is that there is no strong consensus on what the default timeout should be - but this problem persists even if it's pushed to the users of the library. Like, I have my caldav library, and I could consider setting some default timeout there - but then I have the same issue, what would the sane timeout be? This library is again used by other libraries and utilities. Sometimes the end user is the one to learn if the timeout setting is too low or too high. The timeout may be passed through the full chain and be specified by a configuration file for each tool out there - but it adds a lot of complexity. Putting an environment variable there is a simple solution that works for (nearly) all projects. A default timeout given through environment variables may solve quite some problems. It is not a breaking change, users relying on the default timeout to be infinite won't be affected - hence it can be rolled out whenever and without DeprecationWarnings. However, it does not solve this issue. I strongly believe that the default should be set to something else than "infinite" in the long-term future. I've used the requests library a lot, and I always thought that such a high-level library would come with some kind of default timeout ... first time I encountered this issue my script had been hanging for days. I think even a very high default timeout, like 24h, is better than infinite. |
As suggested by `requests`'s documentation [0], define time-outs for http-requests. This change was inspired/suggested by bandit [1][2]. This change addresses all findings of `medium` severity. In addition, switch to using `requests.sessions.Session` where reasonable (this is the case if there is a reasonable likelihood for a session to actually be used more than once). Also, refactor some quirky code. Considerations w.r.t. redundantly defining timeout parameter ------------------------------------------------------------ As discussed on `requests`'s GitHub-Repository (e.g. at [3]), there is no means (except, of course, through "monkey-patching") to define a default timeout, neither globally, nor e.g. for a session. While it is an option to implement a custom transport-adaptor (and use it to inject timeout parameter into issued requests), this approach seems rather heavyweight, and will add a lot of boilerplate code. Therefore, it seems like the best available option to pass `timeout` parameter to each individual invocation of `requests`'s "request-issueing-functions" (request, get, put, ...). One might feel the urge to deduplicate the specified timeout values. However, there are reasons against this: - will also add boilerplate (one extra import stmt) - it is unlikely that timeouts will ever (have to) be changed (more likely: specific individual timeouts might require "tuning" - which is very easy with the chosen approach) Therefore - assuming the chosen de-facto default timeout value proves to be "good enough" - it seems like an adequate choice to redundantly define timeout values. [0] https://requests.readthedocs.io/en/latest/user/advanced/#timeouts [1] https://bandit.readthedocs.io/en/1.7.6/plugins/b113_request_without_timeout.html [2] https://bandit.readthedocs.io/ [3] psf/requests#3070
As suggested by `requests`'s documentation [0], define time-outs for http-requests. This change was inspired/suggested by bandit [1][2]. This change addresses all findings of `medium` severity. In addition, switch to using `requests.sessions.Session` where reasonable (this is the case if there is a reasonable likelihood for a session to actually be used more than once). Also, refactor some quirky code. Considerations w.r.t. redundantly defining timeout parameter ------------------------------------------------------------ As discussed on `requests`'s GitHub-Repository (e.g. at [3]), there is no means (except, of course, through "monkey-patching") to define a default timeout, neither globally, nor e.g. for a session. While it is an option to implement a custom transport-adaptor (and use it to inject timeout parameter into issued requests), this approach seems rather heavyweight, and will add a lot of boilerplate code. Therefore, it seems like the best available option to pass `timeout` parameter to each individual invocation of `requests`'s "request-issueing-functions" (request, get, put, ...). One might feel the urge to deduplicate the specified timeout values. However, there are reasons against this: - will also add boilerplate (one extra import stmt) - it is unlikely that timeouts will ever (have to) be changed (more likely: specific individual timeouts might require "tuning" - which is very easy with the chosen approach) Therefore - assuming the chosen de-facto default timeout value proves to be "good enough" - it seems like an adequate choice to redundantly define timeout values. [0] https://requests.readthedocs.io/en/latest/user/advanced/#timeouts [1] https://bandit.readthedocs.io/en/1.7.6/plugins/b113_request_without_timeout.html [2] https://bandit.readthedocs.io/ [3] psf/requests#3070
As suggested by `requests`'s documentation [0], define time-outs for http-requests. This change was inspired/suggested by bandit [1][2]. This change addresses all findings of `medium` severity. In addition, switch to using `requests.sessions.Session` where reasonable (this is the case if there is a reasonable likelihood for a session to actually be used more than once). Also, refactor some quirky code. Considerations w.r.t. redundantly defining timeout parameter ------------------------------------------------------------ As discussed on `requests`'s GitHub-Repository (e.g. at [3]), there is no means (except, of course, through "monkey-patching") to define a default timeout, neither globally, nor e.g. for a session. While it is an option to implement a custom transport-adaptor (and use it to inject timeout parameter into issued requests), this approach seems rather heavyweight, and will add a lot of boilerplate code. Therefore, it seems like the best available option to pass `timeout` parameter to each individual invocation of `requests`'s "request-issueing-functions" (request, get, put, ...). One might feel the urge to deduplicate the specified timeout values. However, there are reasons against this: - will also add boilerplate (one extra import stmt) - it is unlikely that timeouts will ever (have to) be changed (more likely: specific individual timeouts might require "tuning" - which is very easy with the chosen approach) Therefore - assuming the chosen de-facto default timeout value proves to be "good enough" - it seems like an adequate choice to redundantly define timeout values. [0] https://requests.readthedocs.io/en/latest/user/advanced/#timeouts [1] https://bandit.readthedocs.io/en/1.7.6/plugins/b113_request_without_timeout.html [2] https://bandit.readthedocs.io/ [3] psf/requests#3070
In an effort to clean up the issue tracker to only have issues that are still relevant to the project we've done a quick pass and decided this issue may no longer be relevant for a variety of potential reasons, including:
If you think the issue should remain open, please comment so below or open a new issue and link back to the original issue. Again, thank you for opening the issue and for the discussion, it's much appreciated. |
I still feel strongly that It could cause some minor backwards incompatibility, but fixing this would take requests from freezing your program when used incorrectly to crashing it when used incorrectly, which I consider a fix, not breaking compatibility. I can't get too revved up about this because I'm not the one fixing it, but I do find the absence of a default timeout to be a source of bugs about 20% of the time I see requests in the wild. |
I find made up percentages pulled from no real data to be a source of wasted time 90% of the time |
Well, I think you know what I mean: I see people forgetting to use the |
I have seen the lack of a default timeout in This caused more incidents than there are use cases for not having a timeout (and in this case it can be disabled explicitly). As noted above (almost 7 years ago), this completely disregards the practical needs of users (especially for a library that claims to abstract complexity):
|
maybe another solution would be to warn if there's no timeout, so people can be harassed into providing one, but the default is still the backward compatible "randomly hang forever if one is not provided" |
Earlier in the thread we discussed adding a warning (it's welcome), and after seven years, I don't think anybody in this issue has identified why they would want no timeout at all. The closest we've come is a concern about:
That's it, I think. I don't think this need to be a controversial fix. My personal suggestion would be to implement a fix and add a release note (example below). To me, this is broken functionality, and a default timeout fixes it for users. Maybe I'm missing something, but I don't think I am. If we want to be more conservative, we can take additional steps which add more work, but give people more time and opportunity to adapt:
But I think all of that is unnecessary and that we can just take a middle path:
I really think that would help the vast majority of users and that whatever breakage it caused would be minimal. (Happy to hear contrarian views though!) |
Bandit already warns on lack of a timeout. Anyone with any real care about security of their code should be using it as a static analysis tool. As for a warning Requests emits, people already silence all warnings from the library so they likely wouldn't see a warning about not specifying a timeout |
That's good news, right? That means that the most serious users aren't a concern, and we only have to worry about folks that are less serious, where any breaking impact — if any — would be less of a big deal. I agree most people don't see warnings, so maybe there's not much point in doing them (I know I don't). If that's the case, I still think the upside of just adding a default timeout in the next release is an OK way to go. I appreciate you engaging in this discussion again. If a default timeout is added in the next release, who do you think would be worse off? After all these years and so much engagement, I still can't think of anybody — is this unnecessary conservatism? |
There was a PR to add warnings (unfortunately it was rejected for various reasons): #5434 (comment) The rationale for rejecting the PR actually underscores the need for the warning and a default timeout, as it acknowledges how widespread the issue is (if those projects would be setting proper timeouts, there wouldn't be any warnings):
This is precisely what needs to be addressed, tracking this type of issue down in third-party libraries is difficult, it won't emit any logs, reproducability is limited and it often ends up requiring attaching |
I don't want to pile on, and I know I'm chiming in a lot, but my passion for this issue stems from when I wrote celery tasks that used requests without a timeout. Over time, my celery workers would all slowly get mysteriously tied up, and I'd have to restart Celery. It took me forever to figure out why because I thought it was a celery bug. Nope! I'd have 1000× preferred if my code crashed or timed out. |
This comment has been minimized.
This comment has been minimized.
This adds a default connect and read timeout value for all usage of Requests. This is to solve a long-standing issue where some systems do not have a sufficiently low default value. Personally, I'd want these values to be much lower, but a 10 second connection timeout and a 30 second read timeout seem like they should be enough to avoid problems for the edge cases of users while also not being so large that they're basically ineffective. Closes psf#3070
I have a feeling I'm about to get a swift education on this topic, but I've been thinking about the pros/cons of changing
requests
so that somehow there is a timeout value configured for every request.I think there are two ways to do this:
The reason I'm thinking about this is because I've used requests for a few years now and until now I didn't realize the importance of providing a timeout. It took one of my programs hanging forever for me to realize that the default here isn't really very good for my purposes. (I'm in the process of updating all my code...)
I also see that a lot of people want
Session
objects to have a timeout parameter, and this might be a way to do that as well.If a large default were provided to all requests and all sessions, what negative impact would that have? The only thing I can think of is that some programs will get timeout exceptions where they previously hung, which seems like an improvement to me.
Caveat, added May 13, 2016:
Please don't use this issue to discuss adding a timeout attribute to requests. There are a number of discussions about this elsewhere (search closed issues), and we don't want that conversation to muddy this issue too. Thanks.
The text was updated successfully, but these errors were encountered: