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 the F returned by BlazeClient Kleisli reusable #1792

Merged
merged 1 commit into from Apr 17, 2018

Conversation

Projects
None yet
4 participants
@bryce-anderson
Member

bryce-anderson commented Apr 17, 2018

Fixes #1774

Right now, if you reuse the F[DisposableResponse] returned from the
BlazeClient, it will keep using the same submit time, which doesn't make
sense. To fix it we suspend the entire call body so those important parts
are request specific instead of F specific.

Make the F returned by BlazeClient Kleisli reusable
Right now, if you reuse the F[DisposableResponse] returned from the
BlazeClient, it will keep using the same submit time, which doesn't make
sense. To fix it we suspend the entire call body so those important parts
are request specific instead of F specific.

@bryce-anderson bryce-anderson requested review from rossabaker and SystemFw Apr 17, 2018

@bryce-anderson

This comment has been minimized.

Member

bryce-anderson commented Apr 17, 2018

As noted in #1774, I didn't cook up a test for this. This week my time is pretty limited, so if you want a test someone is free to take over this PR and add it in the likely event that I'm unresponsive. ☹️

@rossabaker

Nice find.

I can't think of a stable way to test this without injecting a Timer into the connection pool, and I can't think of a way to do that without breaking compatibility. I'll file an issue for that.

This looks good in theory and in practice: the repro on #1774 now passes.

@bryce-anderson

This comment has been minimized.

Member

bryce-anderson commented Apr 17, 2018

Are we good to merge this? I typically wait for @ChristopherDavenport to merge things these days. 😄

@jmcardon jmcardon merged commit 7a1dcee into http4s:release-0.18.x Apr 17, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -27,54 +27,56 @@ object BlazeClient {
onShutdown: F[Unit])(implicit F: Sync[F]): Client[F] =
Client(
Kleisli { req =>
val key = RequestKey.fromRequest(req)
val submitTime = Instant.now()
F.suspend {

This comment has been minimized.

@kevinmeredith

kevinmeredith May 3, 2018

Contributor

Could you please explain your usage of F#suspend over F#delay?

This comment has been minimized.

@bryce-anderson

bryce-anderson May 3, 2018

Member

They have different type signatures:

def suspend[A](thunk: => F[A]): F[A]
def delay[A](thunk: => A): F[A]

and suspend fit the bill since we're also generating an F[_] result and just needed to capture even more side effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment