Skip to content

Properly implement Close Handshake #109

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

Closed
nhooyr opened this issue Jul 24, 2019 · 5 comments · Fixed by #110
Closed

Properly implement Close Handshake #109

nhooyr opened this issue Jul 24, 2019 · 5 comments · Fixed by #110

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Jul 24, 2019

See #103

So the problem here is not really that the close handshake is useful but that a lot of libraries expect to be able to reply to a close frame with a echoed close frame and if the connection is closed before they can reply, they error out and it is confusing.

So I'm going to revive my effort in #104 to read the connection automatically in Close() and discard all messages until a Close frame is received for 5s.

This should also go into the README as a nice behavioural advantage over gorilla/websocket and gobwas/ws.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 24, 2019

See https://circleci.com/gh/nhooyr/websocket/16?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link for a test failure that reproduces this.

That test expected an error that indicates the remote WebSocket status frame but instead it got a write failure error out because the remote end closed the connection right after sending the close frame. This is also reproducible with gorilla/websocket and I'm sure a host of other libraries.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 24, 2019

@nhooyr nhooyr added the complex label Jul 24, 2019
@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 24, 2019

Nvm, can't reproduce with gorilla/websocket as it returns the CloseError regardless of whether or not the close frame write succeeded. Maybe we ought to do this too as there is no issue on gorilla/websocket about this and it is much simpler a solution.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 24, 2019

Even if gorilla/websocket gets it right, other WebSocket libraries might not so I will need to survey them.

@nhooyr
Copy link
Contributor Author

nhooyr commented Jul 24, 2019

I say we just keep it simple for now, if it ends up being an issue, I'll keep that branch so we can refer to it and easily implement it.

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

Successfully merging a pull request may close this issue.

1 participant