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 overall timeout support #936

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add overall timeout support #936

wants to merge 9 commits into from

Conversation

karpetrosyan
Copy link
Member

@karpetrosyan karpetrosyan commented Jul 13, 2024

Todo

  • Update documentation

Hi! It seems we have received many requests for adding total timeout support.

In each place where we are using the operation (read, write, pool, connect) timeout, we should take the minimum value of the operation timeout (timeouts['pool'], for example) and the total timeout value (timeouts['total']). We should also measure the time of each operation and adjust the value of the total timeout accordingly, so we always have the correct total timeout value within the scope of the request.

I'v used the context manager syntax to simplify time measuring, also have added a simple api level test for total timeout, as we don't have any tests for the concrete timeouts

@karpetrosyan karpetrosyan requested a review from tomchristie July 14, 2024 07:33
@karpetrosyan
Copy link
Member Author

@tomchristie Any thoughts?

@tomchristie
Copy link
Member

Thanks @karpetrosyan...

Any thoughts?

  • This is a fantastic piece of functionality to add yes, much needed.
  • Could we get a docs update.
  • Related work that's become apparent to me is that read/write timeouts are the wrong API... a single socket timeout would be a better fit.

docs/extensions.md Outdated Show resolved Hide resolved
@karpetrosyan
Copy link
Member Author

I guess that's all from our side. The remaining documentation will be available in the HTTPX docs.

@gsakkis
Copy link

gsakkis commented Aug 28, 2024

@karpetrosyan this looks great! Unfortunately this doesn't quit solve my use case as I want to exclude pool timeout from total. How hard would it be make the definition of total customizable? Potential API off the top of my head:

r = httpcore.request(
    "GET",
    "https://www.example.com",
    extensions={"timeout": {
        "read": 1.0, 
        ("connect", "read", "write"): 3.0,
        ("connect", "read", "write", "pool"): 10.0,  # equivalent to "total"
    }}
)

@brosoul
Copy link

brosoul commented Oct 16, 2024

I'm really looking forward to this feature. Could you please ping me after PR is ready?

@gsakkis
Copy link

gsakkis commented Oct 16, 2024

@brosoul there is a "subscribe" button at the top right of the page, click it to get notified for updates to this issue.

@tomchristie
Copy link
Member

Thanks so much for your work on this @karpetrosyan.

I've been spending a little time thinking about this and I reckon there's a more fundamental improvement we could be making here. Design discussion over at #982.

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

Successfully merging this pull request may close these issues.

4 participants