Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.1k
If WebSocket closes abnormaly send status code 1006 to app#12900
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Draft
mkurz wants to merge4 commits intoplayframework:mainChoose a base branch frommkurz:websocket_status-code_1006
base:main
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or 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
I guess the close messages in that file are handled by the flow as if sent by user code - however they are not really sent by the user, so should we also pass 1006 to the app for connections that get closed triggered from that code? |
Sign up for freeto 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.
SeeWebSocket specs 7.4.1
Also7.1.5
And11.7
The problem is right now the app only gets a status code if the connection gets closed correctly, triggered by the client. (If user code triggers closing, no status code will be send to the app which is OK).
However, if a WebSocket connection is lost / abnormaly closed, for whatever reason (network is down, cable unplugged, ...), currently the app does not receive a close message, but according to the specs this is what 1006 is for.
Currently, one can only detect if a connection is closed via watchTermination on a flow like (java code):
As you see from this code, the watchtermination is always called in a way so that done is set, no matter what the reason is that a connections got closed: you can not determine the connection was closed cleanly (triggered by user code or by the client) or if the connection was closed abnormally. There is only one work around I can think of currently, which would be to capture the close status code of a close message, if one is received, and check in the watch termination if there was a close message before, and if so one knows if the connection was closed clean or not (also the user needs to save if the connection was closed by user code of course to make the distinction).
With this PR I always make the WebSocket send a close status code to the user code with status 1006 if the connection is not closed cleanly by user code nor by the client.