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

fix(ws): Don't send two close frames #22684

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jespertheend
Copy link
Contributor

Fixes #21642

@jespertheend
Copy link
Contributor Author

@mmastrac could you check if this is the correct way to go about this?

The gist is that the fastwebsockets crate responds with a close frame whenever it receives a close frame. But this includes when the websocket was closed from Deno's end. As a result, two close frames are sent and Chrome logs an error in the console. So basically what happens is:

  1. Deno sends a close frame
  2. Client responds with a close frame
  3. Deno responds with a close frame again

This PR prevents any frames from being sent once the WebSocket is closed. But I worry this might be a bit too extreme. I tried fixing it in the fastwebsockets crate, but couldn't find an easy way to check whether the websocket was closed or not.

Let me know if there's a better way to do this. And if not, I'll start adding tests.

@littledivy
Copy link
Member

@jespertheend fastwebsocket has set_auto_close() to control whether to send a close frame automatically: https://github.com/denoland/fastwebsockets/blob/5402e4eacdaebd82856e4721151e522bff00e04f/src/lib.rs#L438

@littledivy
Copy link
Member

I think the bug is that we are not closing the TCP connection immediately after sending the close frame.

https://datatracker.ietf.org/doc/html/rfc6455#section-5.5.1

The server MUST close the underlying TCP connection immediately;

@jespertheend
Copy link
Contributor Author

think the bug is that we are not closing the TCP connection immediately

Ah I see, that makes sense. I'll look into that!

@jespertheend
Copy link
Contributor Author

Hah! Turns out this was a lot easier than I had expected. I was trying to find a way to pass the created TcpStream to fastwebsockets somehow, but it seems like calling shutdown() on the write_half works as well.
I'll open up a PR for fastwebsockets shortly.

@jespertheend
Copy link
Contributor Author

I've submitted denoland/fastwebsockets#72

@bartlomieju
Copy link
Member

@littledivy can we update fastwebsocket and merge this PR?

@jespertheend
Copy link
Contributor Author

Once denoland/fastwebsockets#72 is fixed and merged, this PR would no longer be necessary.

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

Successfully merging this pull request may close these issues.

Closing WebSockets from the server results in 'Close received after close' error
3 participants