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

Make HTTP/1.1 implementation respect read_timeout configuration #8

Closed
mtrudel opened this issue Oct 12, 2021 · 4 comments
Closed

Make HTTP/1.1 implementation respect read_timeout configuration #8

mtrudel opened this issue Oct 12, 2021 · 4 comments
Labels
enhancement New feature or request http/1

Comments

@mtrudel
Copy link
Owner

mtrudel commented Oct 12, 2021

The HTTP/1.1 stack doesn't currently respect the top-level read_timeout parameter. This comes down mostly to passing it into Socket.recv calls and returning it on keepalive loops. For testing prior art, see https://github.com/mtrudel/bandit/blob/main/test/bandit/http2/protocol_test.exs#L62

@mtrudel mtrudel added enhancement New feature or request http/1 labels Oct 12, 2021
@mtrudel mtrudel added this to the 0.4.x - Comprehensive HTTP/1.1 review milestone Oct 12, 2021
@afsharalex
Copy link

Hi, would this be a good first issue to tackle (if no one else is already)?

@mtrudel
Copy link
Owner Author

mtrudel commented Apr 25, 2022

Hi! Thanks for the offer of help!

I've been thinking that this timeout work (as well as the existing HTTP2 timeout work referenced above) belong better as a configurable parameter in the underlying socket as implemented in Thousand Island. Having to carry a timeout value around in Bandit's state seems the wrong pace to do it when the ThousandIsland.Socket island struct is already carrying the underlying socket around (and is, by necessity, the API through which ALL requests to the underlying socket pass so it's a natural place to apply such a timeout).

If you're game for taking that work on it would be much appreciated. I've written up my initial thoughts on the matter at mtrudel/thousand_island#15

Once that work lands, the scope of this ticket can change to actually remove timeout code from the HTTP/2 side, since we'll be able to handle both cases intrinsically using the new Thousand Island timeout feature.

WDYT?

@afsharalex
Copy link

Awesome, sounds good!

I'd love to help out with Thousand Island as well :)

Thanks!

@mtrudel
Copy link
Owner Author

mtrudel commented Sep 20, 2022

This landed back in 46622b7 and I neglected to close this issue

@mtrudel mtrudel closed this as completed Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request http/1
Projects
None yet
Development

No branches or pull requests

2 participants