Listen to CloseNotifier in stream handler. #382
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem:
context.Stream() already listens to the closeNotifier, which gives the impression that context.Stream() will return as soon as a client disconnects. Unfortunately, it doesn't!
This example code furthers the impression with the defer closeListener(roomid, listener) which one would expect to be called when a client navigates away, which is not the case. In fact, it's only called on the next message that's sent to the room after said client exits.
This is because c.SSEvent("message", <-listener) blocks indefinitely until a message comes in, so will not return and yield to c.Stream()'s select that catches the closeNotify. This means that if a client navigates away, the server never notices and cleans up.
Fix: Also listen to CloseNotifier inside stream handler. This causes the step() function to return immediately when the client goes away and allows the cleanup to run. Updating this example should make it more clear, I think!
|
Looks nice |
appleboy
approved these changes
May 31, 2018
thinkerou
approved these changes
Aug 22, 2018
|
@appleboy can you merged the pr? |
|
@thinkerou I can't merge and update the branch. @javierprovecho Please help to merge. |
This was referenced Mar 1, 2019
|
@spynup thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem:
context.Stream() listens to the closeNotifier, which gives the impression that context.Stream() will return as soon as a client disconnects. Unfortunately, it doesn't!
This example code furthers the impression with the defer closeListener(roomid, listener) which one would expect to be called when a client navigates away, which is not the case. In fact, it's only called on the next message that's sent to the room after said client exits.
This is because c.SSEvent("message", <-listener) blocks indefinitely until a message comes in, so will not return and yield to c.Stream()'s select that catches the closeNotify. This means that if a client navigates away, the server never notices and cleans up.
Fix: Also listen to CloseNotifier inside stream handler. This causes the step() function to return immediately when the client goes away and allows the cleanup to run. Updating this example should make it more clear, I think!