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

receive_timeout regarding stream/5 & async_request/3 #243

Closed
jswanner opened this issue Aug 30, 2023 · 6 comments
Closed

receive_timeout regarding stream/5 & async_request/3 #243

jswanner opened this issue Aug 30, 2023 · 6 comments

Comments

@jswanner
Copy link
Contributor

Thanks for the amazing library!

Finch.stream/5 seems to treat the receive_timeout option differently between v0.16.0 & main (9609c40), and I believe it stems from 823cc65.

On main, Finch.stream/5 (and Finch.async_request/3) timeout after the receive_timeout threshold has elapsed, even if data are being received. While on v0.16.0, the connection times out after the receive_timeout threshold has elapsed since the last chunk of data (rather than from the start of the connection).

Maybe I'm wanting to do something that's not intended to be supported, which is to leave the streaming connection open as long as it's still receiving data (the API I'm calling should send pings every 5 seconds). I can work around the problem by setting a very high receive_timeout value and then reconnect on timeout, but it would be nice to somehow have the receive_timeout behavior of v0.16.0.

@sneako
Copy link
Owner

sneako commented Aug 30, 2023

Thank you for the kind words!

I see your point.

I guess, the value that is passed to the recv calls should always be the :receive_timeout and something similar to the new behaviour that was introduced could be implemented as a new :request_timeout option for the overall duration of request.

@jswanner
Copy link
Contributor Author

I guess, the value that is passed to the recv calls should always be the :receive_timeout and something similar to the new behaviour that was introduced could be implemented as a new :request_timeout option for the overall duration of request.

@sneako, what you proposed sounds good to me. I thought I understood what needed to happen well enough to implement it myself, but turns out I don't know what I'm doing. Mint seems to be pipelining requests, and the request ref is unexpectedly changing.

Would you like me to push a broken PR? Maybe you'll have some insight into what I'm doing wrong.

@zachallaun
Copy link
Contributor

Would you like me to push a broken PR? Maybe you'll have some insight into what I'm doing wrong.

I'm not @sneako, but it seems totally reasonable to push it as a Draft PR!

jswanner added a commit to jswanner/finch that referenced this issue Sep 1, 2023
Intended to timeout a request with a chunked response after the elapsed
time has surpassed the given value. See sneako#243.
@jswanner
Copy link
Contributor Author

jswanner commented Sep 1, 2023

Thanks for the prompt @zachallaun, I had been intending to make the PR, but had been focusing on other things.

jswanner added a commit to jswanner/finch that referenced this issue Sep 10, 2023
jswanner added a commit to jswanner/finch that referenced this issue Sep 10, 2023
jswanner added a commit to jswanner/finch that referenced this issue Sep 24, 2023
Intended to timeout a request with a chunked response after the elapsed
time has surpassed the given value. See sneako#243.
jswanner added a commit to jswanner/finch that referenced this issue Nov 7, 2023
Intended to timeout a request with a chunked response after the elapsed
time has surpassed the given value. See sneako#243.
@sneako
Copy link
Owner

sneako commented Nov 8, 2023

Closed via #244

@sneako sneako closed this as completed Nov 8, 2023
@jswanner
Copy link
Contributor Author

jswanner commented Nov 8, 2023

Thank you, @sneako! ❤️

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

3 participants