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

feat(client): allow to set response timeout per request #1170

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joelwurtz
Copy link
Contributor

@joelwurtz joelwurtz commented Dec 26, 2024

This allow configuring all timeout options per request instead of having only the request timeout set (obviously connect / tls timeout may not be used each time will depend on the pool)

EDIT : PR changed to only specifying request / response timeout instead of all possible one

@fakeshadow
Copy link
Collaborator

IMO API like this is confusing to users who don't know the low level of connection well. It may not result in a behavior they desire due to how connection pool work.

There seems no real usage where you need a scoped timeout rather than request/response timeout. The connect and handshake timeout are there for eventual file descriptor recycle to avoid keep hogging system resources and the slowest connection you make determines the value of it. Shrinking/prolonging the duration does not make a difference.

@joelwurtz
Copy link
Contributor Author

joelwurtz commented Jan 6, 2025

Main goal was to reuse the same client for different host with different connect timeout, but we can always use different client in this case you are right.

Then i can reduce the timeout config to only request / response ? ATM, on main, it's only request, response timeout depend on the client.

EDIT : changed to only request / response timeout per request builder

@joelwurtz joelwurtz force-pushed the feat/client-timeout branch 4 times, most recently from 60e8190 to 7ffede1 Compare January 6, 2025 10:24
@joelwurtz joelwurtz changed the title feat(client): allow to set timeout per request feat(client): allow to set response timeout per request Jan 6, 2025
@joelwurtz joelwurtz force-pushed the feat/client-timeout branch 2 times, most recently from 8a8c0d4 to 63e8346 Compare February 5, 2025 16:22
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.

2 participants