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

Fixed connection release when body isn't run, as well as thread affinity #3261

Closed

Conversation

djspiewak
Copy link
Contributor

@djspiewak djspiewak commented Mar 19, 2020

This PR fixes three issues in the async-http-client implementation:

  • When the body of the response was never sequenced, the upstream connection would never be closed and would ultimately end up stuck in TCP CLOSE_WAIT
  • The fiber continuation of the Response was run on an AHC thread, rather than on the pool governed by the relevant ContextShift. In order to avoid breaking bincompat, this was fixed by forking a fiber to run the async callback
  • Under certain circumstances, when the body of an HTTP was empty, the prior implementation could deadlock. This is fixed by adding a callback to onComplete. Note that, by law, Cats Effect async callbacks ignore repeated invocations, so it's safe to put this here without a latch to guard it.

It's worth noting that there is still a known issue with this fix: namely, if you start running the body stream and then the controlling fiber is almost immediately canceled, then the connection can leak. This can happen because we cannot uninterruptibly call subscribe and start the stream (which would register the finalizer that runs cancel). The only way to fix this is to change fs2-reactive-streams' .stream function to take an F[Unit] representing the subscription effect, which it would then sequence and manage itself in a properly bracketed fashion.

This case is very unlikely, and at least things are better than they were.

Copy link
Member

@rossabaker rossabaker left a comment

Nice work.

Since this is the binary compatible fix, we should backport this to series/0.21.

@rossabaker rossabaker added this to the 0.21.2 milestone Mar 20, 2020
@rossabaker rossabaker added the bug label Mar 20, 2020
hamnis
hamnis approved these changes Mar 20, 2020
rossabaker pushed a commit that referenced this issue Mar 21, 2020
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Mar 21, 2020

Cherry-picked onto series/0.21.

@rossabaker rossabaker closed this Mar 21, 2020
rossabaker added a commit to rossabaker/http4s that referenced this issue Mar 21, 2020
armanbilge pushed a commit to http4s/http4s-async-http-client that referenced this issue May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants