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

Support cancellation in blaze-server on stage shutdown #2063

Merged
merged 4 commits into from Nov 5, 2018

Conversation

Projects
None yet
2 participants
@rossabaker
Member

rossabaker commented Sep 8, 2018

When a blaze server stage is shut down, we should be able to cancel any running request, because it's not going to render anyway. This is similar to a servlet response being canceled when the AsyncContext timeout fires.

@@ -253,12 +257,21 @@ private[blaze] class Http1ServerStage[F[_]](
override protected def stageShutdown(): Unit = {
logger.debug("Shutting down HttpPipeline")
parser.synchronized {
cancel()

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Sep 8, 2018

Member

Does this signal cancelled even on correct shutdown?

This comment has been minimized.

@rossabaker

rossabaker Sep 8, 2018

Member

I think it would. We could blank it out, but I had to synchronize on the parser to make it reliable, and we'd have to think through the deadlock ramifications.

Alternatively: make the test pass without synchronization.

This comment has been minimized.

@rossabaker

rossabaker Sep 24, 2018

Member

Detecting correct shutdown is going to be tricky here. Maybe it's more appropriate to cancel only when we receive a Disconnected message?

@rossabaker rossabaker modified the milestones: 0.19.0-M3, 0.19.0 Sep 23, 2018

@rossabaker rossabaker modified the milestones: 0.19.0, 0.19.0-RC1 Oct 3, 2018

@ChristopherDavenport

This comment has been minimized.

Member

ChristopherDavenport commented Oct 8, 2018

Want to incorporate the Stream change?

@rossabaker

This comment has been minimized.

Member

rossabaker commented Oct 8, 2018

Which stream change?

@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 4, 2018

@ChristopherDavenport I'm still confused as to which stream change.

@ChristopherDavenport

This comment has been minimized.

Member

ChristopherDavenport commented Nov 5, 2018

That was from a conversation on gitter. I do not think it is relevant.

@rossabaker rossabaker merged commit 4309b1c into http4s:master Nov 5, 2018

2 checks passed

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

@rossabaker rossabaker deleted the rossabaker:blaze-server-cancellation branch Nov 5, 2018

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