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

read timeout values from config (or fallback to default) #25

Merged
merged 2 commits into from
Dec 23, 2022

Conversation

dtrifiro
Copy link
Contributor

@dtrifiro dtrifiro commented Dec 21, 2022

Timeouts can be configured using the connect_timeout, sock_connect_timeout and sock_read_timeout kwargs.

Documentation for these can be found in the aiohttp docs: https://docs.aiohttp.org/en/stable/client_reference.html?highlight=clienttimeout#clienttimeout.

Setting these to 0 or None disables the timeouts entirely.

closes #15

@dtrifiro dtrifiro marked this pull request as ready for review December 21, 2022 14:32
@dtrifiro dtrifiro requested a review from efiop December 21, 2022 14:59
@efiop efiop requested a review from skshetry December 21, 2022 16:13
@skshetry
Copy link
Member

Before adding config, I think we should research why the timeout is occurring, because this does not solve the default use case (timeout is an escape hatch, but very few will know about this).

The last time I checked, it seemed like a bug on how aiohttp considered timeout for streaming/chunked downloads/uploads.

@daavoo
Copy link

daavoo commented Dec 22, 2022

Before adding config, I think we should research why the timeout is occurring, because this does not solve the default use case (timeout is an escape hatch, but very few will know about this).

Does it hurt to have the config?
We could at least suggest to users facing the problem a workaround while we research where the timeout comes from?

@dtrifiro
Copy link
Contributor Author

@skshetry I agree. I'm researching the cause of the timeout, but I think it's worth to have this configurable by users in order to let users avoid failures with push on http remotes. It's only a workaround, but at least this will unblock some users.

@dtrifiro dtrifiro merged commit fe2ae8e into main Dec 23, 2022
@dtrifiro dtrifiro deleted the set-timeouts-from-config branch December 23, 2022 09:56
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.

expose connect_timeout on HTTP remote
3 participants