elixir-mint / mint

Functional HTTP client for Elixir with support for HTTP/1 and HTTP/2 🌱
Apache License 2.0
1.36k stars 106 forks source link

Prevent open requests from leaking when HTTP2.request or HTTP2. stream_request_body return an error #326

Closed mezz closed 2 years ago

mezz commented 2 years ago

Hello, and thank you for the great library!

Issue Description

HTTP2 leaks open requests in some cases when there are errors raised in HTTP2.request/5.

Background

I ran into this when hitting :exceeds_window_size and :too_many_concurrent_requests errors in production. We get some of these errors during the normal operation of our service, and each one leaks a request. When this happens, we retry our request on a different HTTP2 connection, so the leak was not noticeable for a long time.

Impact of the issue

Unfortunately we can eventually end up with enough leaked requests so that every HTTP2 request will error with :too_many_concurrent_requests. This leaves the HTTP2 connection in a bad state where it will no longer work.

PR Changes

This PR introduces some logic to clean up these errored requests, and adds a few checks to the tests to confirm the fix.

ericmj commented 2 years ago

Thanks for finding the issue and sending a PR with a fix! I think we need to do the same fix in stream_request_body.

mezz commented 2 years ago

I have:

To test that transport errors are handled here, I created a TestTransportSendTimeout module which is the same as the SSL transport module except it always times out on successful sends. The way I use it in the tests seems a bit less clean than would be ideal, but there is currently no way to use a transport other than TCP and SSL and I didn't want to go down that rabbit hole in this PR. Let me know if that works for you or if there's a better approach you'd like to use for these.

ericmj commented 2 years ago

Thank you! 💜