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

Don't start response until after we add async timeout listener #2106

Merged
merged 3 commits into from Sep 24, 2018

Conversation

Projects
None yet
4 participants
@rossabaker
Member

rossabaker commented Sep 24, 2018

It is an error to add an async context listener after a context is completed. We need to install the timeout listener before we race against the response.

The unit test passed before the fix, but shows that we don't break the basic functionality. The error condition is rendered on an asynchronous, container owned thread, and I don't see that we have access to it. Furthermore, it only happens under load.

Fixes #2103.

@rossabaker rossabaker added the bug label Sep 24, 2018

@rossabaker rossabaker added this to the 0.19.0-M3 milestone Sep 24, 2018

@rossabaker rossabaker requested a review from SystemFw Sep 24, 2018

@rossabaker

This comment has been minimized.

Member

rossabaker commented Sep 24, 2018

This is a Travis-only failure. It passes locally.

@SystemFw

Better to get a third review in since we talked about this together

@rossabaker

This comment has been minimized.

Member

rossabaker commented Sep 24, 2018

Appveyor error is unrelated.

@rossabaker rossabaker merged commit deac89c into http4s:master Sep 24, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rossabaker rossabaker deleted the rossabaker:issue-2103 branch Sep 24, 2018

.suspend(serviceFn(request))
.getOrElse(Response.notFound)
.recoverWith(serviceErrorHandler(request))
gate.get *>

This comment has been minimized.

@YulawOne

YulawOne Sep 24, 2018

Sorry, maybe I'm missing something, but I don't see difference between race with synchronization on gate here and plain flatMap, i.e. timeout *> response because we will wait gate.get completion

This comment has been minimized.

@rossabaker

rossabaker Sep 24, 2018

Member

This makes response wait on the call to ctx.addListener, not the asynchronous completion of timeout. The original bug was because response could complete ctx before ctx.addListener was called. And timeout *> response would wait for the timeout to fire before attempting response.

This comment has been minimized.

@YulawOne

YulawOne Sep 24, 2018

Sorry, my brain have completely ignored part with F.asyncF[Response[F]](cb =>...), and now with your explanation i've finally understood whole picture. Thanks for explanation!

This comment has been minimized.

@rossabaker

rossabaker Sep 24, 2018

Member

No problem. Thanks for the bug report! This should be released in 0.19.0-M3 as soon as the fs2 release candidate is out.

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