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

Avoid incorrectly responding with an empty body in AHC client #3338

Merged
merged 4 commits into from Apr 23, 2020

Conversation

wemrysi
Copy link
Contributor

@wemrysi wemrysi commented Apr 19, 2020

  1. Guards the callback invocation in onCompleted so that it is only called if onStream wasn't called.
  2. Fixes bodyDisposal to actually sequence the stream finalizer via .pull.uncons.void. take(0) turns out to be a noop, so the previous version wasn't finalizing.

Fixes #3293

Copy link
Member

@rossabaker rossabaker left a comment

This seems reasonable. I'm rerunning the tests, and opened a PR to disable the unrelated flaky test that tanked the first run. We'll see if this runs green again.

We'll want to cherry-pick this back to series/0.20.

/cc @djspiewak, who has thought harder about this code than I have.

override def onCompleted(): Unit =
invokeCallback(logger)(cb(Right(response -> dispose)))
onStreamCalled.get
.ifM(F.unit, F.delay(invokeCallback(logger)(cb(Right(response -> dispose)))))
Copy link
Member

@rossabaker rossabaker Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F.delaying something that calls unsafeRunSync to be called in an unsafeRunSync feels abusive, even for a spot we need Effect. I'd like to think harder about this, but more importantly, I'd like to merge the bugfix.

Copy link
Member

@aeons aeons Apr 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also one of the places where using the ifTrue and ifFalse labels helps readability a lot.

@rossabaker rossabaker added the bug label Apr 20, 2020
@rossabaker rossabaker added this to the 0.21.4 milestone Apr 20, 2020
hamnis
hamnis approved these changes Apr 20, 2020
@wemrysi
Copy link
Contributor Author

@wemrysi wemrysi commented Apr 20, 2020

Appreciate the feedback, I've pushed another commit addressing the comments.

bodyDisposal <- Ref.of[F, F[Unit]] {
subscriber.stream(subscribeF).take(0).compile.drain
subscriber.stream(subscribeF).pull.uncons.void.stream.compile.drain
Copy link
Contributor

@kevinmeredith kevinmeredith Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain what "force finalization" means, @wemrysi?

Copy link
Contributor Author

@wemrysi wemrysi Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinmeredith in our usage, we noticed that if the body of responses produced by the AHC client weren't consumed, the connection was never closed. A solution to this was to add an effect to the Resource finalizer that would subscribe to the body stream and then immediately cancel the subscription, which closes the connection.

The original attempt at this used take(0) which turns out to be a noop (it results in a pure, empty stream) and thus the subscribe/cancel effects are never run. Switching to pull.uncons ensures that the first effect in the stream is sequenced and any finalizers are run when the stream ends.

Copy link
Contributor

@kevinmeredith kevinmeredith Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wemrysi!

@rossabaker rossabaker merged commit f4ef8d9 into http4s:master Apr 23, 2020
8 checks passed
@wemrysi wemrysi deleted the bug/3293-fix-ahc-empty-body branch Apr 23, 2020
@rossabaker rossabaker added the retarget label Apr 26, 2020
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 26, 2020

Oops, we merged this to master. Cherry-picking. I think it may fix #3354.

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 retarget
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants