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

Send Disconnect event on EOF #2061

Merged
merged 1 commit into from Sep 24, 2018
Merged

Conversation

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Sep 8, 2018

When we read an EOF, request a disconnect. This signals outbound that we're done and allows a chance to clean up.

Before this change, when a client disconnects without either the server or client signalling the end of the connection, the HTTP stage is shut down but no command is passed out. This delays the cleanup of the middle stages, such as QuietTimeoutStage.

Copy link
Member Author

@rossabaker rossabaker left a comment

I don't consider this change critical for 0.19.0-M2, but I want to establish that we're adapting correctly to blaze's evolution.

@@ -87,7 +87,7 @@ private[blaze] class Http1ServerStage[F[_]](

private val handleReqRead: Try[ByteBuffer] => Unit = {
case Success(buff) => reqLoopCallback(buff)
case Failure(Cmd.EOF) => stageShutdown()
case Failure(Cmd.EOF) => closeConnection()

This comment has been minimized.

@rossabaker

rossabaker Sep 8, 2018
Author Member

Idiom question for blaze-0.14: should we be shutting down here, or waiting for a Disconnected message to find its way back to us?

@rossabaker rossabaker force-pushed the rossabaker:send-disconnect branch from 5c17f12 to 99ac22a Sep 8, 2018
@rossabaker rossabaker merged commit a000b3a into http4s:master Sep 24, 2018
2 checks passed
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:send-disconnect branch Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants