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

User guide for how to provide timeouts #5789

Closed
torotil opened this issue Apr 8, 2021 · 11 comments
Closed

User guide for how to provide timeouts #5789

torotil opened this issue Apr 8, 2021 · 11 comments

Comments

@torotil
Copy link

torotil commented Apr 8, 2021

As many others before me I have the need to set a default timeout for all requests (blocking forever just isn’t a very good default in most cases I guess).

I appreciate that this has been covered exhaustingly in this issue queue, that there might not be a recommendation for all use-cases, and there are different ways to do this.

That’s exactly why I’m proposing to add documentation at the appropriate places for this in order to improve the experience for downstream developers and to avoid further mayhem in the issue queue.

So what are recommendable ways to have a default timeout and which disadvantages / advantages do they have?

@sigmavirus24
Copy link
Contributor

All of your issues describe providing one timeout for a session rather than how to provide timeouts for requests.

It sounds like you thus want to add an example of a recipe to the documentation that shows people how to create their own session object that accepts a timeout and passes that on to every request the session makes? Is that accurate? If not, can you please explain (perhaps with an example) what you're thinking needs to be added exactly?

@torotil
Copy link
Author

torotil commented Apr 9, 2021

All of your issues describe providing one timeout for a session rather than how to provide timeouts for requests.

In more general terms it’s mostly about pre-configuring request parameters (eg. authentication, headers, …) so that not every part of the code initiating a request needs to care about all these details. For most of these parameters sessions can be used for exactly that, which is perhaps the reason why people tend to think about putting the timeout there as well. I actually don’t mind what the concept of encapsulating these parameters is called. What matters is that it accomplishes the encapsulation and is easy to use.

what you're thinking needs to be added exactly?

I’d suggest to:

  1. Add a note to the session documentation and maybe even the API-docs that although it works for most parameters it doesn’t work for timeout not by obmission but on purpose (maybe referencing an appropriate issue where this has been discussed).
  2. Include 1 or 2 recipies on how to achieve a session-like encapsulation for timeouts so that the Python community doesn’t end up with 1001 solutions to more or less the same problem. I probably would put those in the timeout section and link them from the above notes.

So the open questions for me are:

  • Does adding this to the docs sound sensible?
  • Which recipies are worth putting there?

@sigmavirus24
Copy link
Contributor

Include 1 or 2 recipies on how to achieve a session-like encapsulation for timeouts so that the Python community doesn’t end up with 1001 solutions to more or less the same problem. I probably would put those in the timeout section and link them from the above notes.

Absolutely not.

Ignoring the mounds of hate mail I've received for a decision I didn't make, but frankly agree with, this would then make it seem like "the library authors encourage this but won't support it" which will cause a flood of new issues and pull requests for something that we won't be adding.

If there are other improvements around session attributes you can point out that you think are missing, I'm happy to review a PR adding those

@torotil
Copy link
Author

torotil commented Apr 10, 2021

Ignoring the mounds of hate mail I've received for a decision I didn't make

I’m really sorry to hear you are getting hate mail for any decision made in an open source project. I definitely like to avoid that. I’m also not suggesting to revise that decision. I’m trying to find ways to implement it.

Do you also disagree with 1.? (making this decision visible in the docs)


this would then make it seem like "the library authors encourage this but won't support it"

I think the “this“ is important in this sentence: Do you think there is absolutely no sensible way to achieve an encapsulation of the timeout that fits the decision & the design of the library? … even for specific use-cases? — For me it seems that at least this comment includes a way to do this that doesn’t bind the timeout to the session at all.


I imagine most developers who end up bringing this to the issue queue go through a process roughly like this:

  • I need to add a timeout to all of my requests (otherwise they might block forever). Is there a way to configure this at one place [instead of needing to pass it with each request]? → Consulting the docs.
  • The docs say the Session can be used to pre-configure most of the request params. → Let’s try if this works for the timeout as well.
  • This doesn’t work. It seems like an oversight. → Let’s post an issue.
  • Ok, there is a closed issue about this that refers to duplicates. → Let’s see if those contain something useful.
  • Wow, there is many possible ways, but the maintainers seem not to be convinced by any of them. → What should I do now?

I think adding something to the docs would catch them before they are too invested in the idea that the Session is the right place to add this. Pointing them to a recipy that solves their problem would avoid them coming to the issue queue altogether — even if it’s a “We don’t recommend this, but some users have found this useful.”

@jakermx
Copy link

jakermx commented Apr 12, 2021

Hi there,

Remember this lib , is just a user-friendly or elegant HTTP Requests implementation , based on urllib3 , so...it depends on urllib and it depends on socket lib....if you are loking for a more deep implementation, I would suggest you to use adapters and look for tcp session params on python..you can set session and connection timeouts, while urllib release a easy way to set then.

Cheers

@jakecobb
Copy link

It seems reasonable that this should be emphasized more in the docs since it won't be supported directly. Quickstart > Timeouts is the second to last section and contains this gotcha:

Nearly all production code should use this parameter in nearly all requests. Failure to do so can cause your program to hang indefinitely

The default behavior blocks forever under relatively common and transient network conditions, and almost every example in the documentation outside of the Timeout section itself shows this usage.

If a default timeout does not belong to the Session object philosophically but instead to transport, couldn't it be added as an example under Transport Adapters where there is already an example of other non-default behavior?

Or perhaps the other Quickstart examples should always be passing the timeout parameter explicitly so the importance of its use is emphasized.

@0xallie
Copy link

0xallie commented Nov 26, 2021

You say timeout doesn't belong to the Session object conceptually, OK. But then why not add a timeout parameter to HTTPAdapter? It would be much better than saying "just add this 10 line boilerplate to every project where you need to use session-level timeouts". From the amount of duplicate issues about this it should be clear that this is not a niche use case but is quite common. If there is any reason that wouldn't make sense please enlighten me, because I'm struggling to understand.

It also seems completely self-contradictory for the documentation to suggest that "nearly every request should have a timeout" while the library doesn't provide a convenient way to specify a default timeout rather than repeating it on every request, and neither does the documentation provide a recipe for subclassing HTTPAdapter to add this functionality. Please explain the logic behind this, because I would think for the overwhelming majority of use cases a session-level timeout with maybe a couple of manual overrides would work fine and make things much easier. (Whether it's on the session itself or the adapter is just an implementation detail and not my point here.)

If adding it to HTTPAdapter itself isn't a workable solution for whatever reason, maybe it could go in requests-toolbelt or a separate PyPI package (though the former seems unmaintained, and the latter would encourage Node.js-style dependency creep if there's a package just for Requests session timeouts).

@torotil
Copy link
Author

torotil commented Dec 2, 2021

Remember this lib , is just a user-friendly or elegant HTTP Requests implementation …

Exactly. And that’s why it’s worth looking for a user-friendly way to support a very common use-case — I daresay code that needs a timeout is more common than code that doesn’t. So perhaps we are talking about nothing less than the most common use case for this library and how to make it less error prone.

@kkirsche
Copy link
Contributor

Potentially instead of a changing the timeout property / attribute itself, would it make sense to offer the ability to warn when no timeout is set, similar to how more recent python versions can warn about using open without an encoding value?

@jakub-m
Copy link

jakub-m commented Feb 15, 2024

Hi, let me revive the discussion. I stumbled upon the "default timeout" issue recently. I think that a very simple solution, like global default timeout that is applied to all the requests when no more specific timeout is provided, would address many operational pains. I would imagine that such a timeout could be set with some requests.set_default_timeout() or with pyproject.toml , to really apply to all the requests that use (awesome) requests library. The real-world return from such an investment in the code would be huge.

I'd happily provide a PR for such a feature, I'd like to know though which direction would be the best one.

@torotil
Copy link
Author

torotil commented Feb 15, 2024

This is one of the reasons why I would use httpx which has an easy to use solution for timeouts for new projects. For my current projects I’d hope to migrate at some point.

I’m closing this issue now as I don’t have hopes that the maintainers will agree to improvements of either the code or the documentation of the library before I migrate.

@torotil torotil closed this as completed Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants