Skip to content

fix(ext/websocket): extra ws pongs sent #17762

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

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

littledivy
Copy link
Member

Fixes #17761

Tugstenite already sends a pong for a recieved ping. This automatically happens when the socket read is being driven. From snapview/tokio-tungstenite#88

You need to read from the read-side of the socket so that it receives/handles pings, and on the next write it would then send the corresponding pong.

Here's the source:

https://github.com/snapview/tungstenite-rs/blob/e1033afd959bb7abfcbc181033b8326f8a40562b/src/protocol/mod.rs#L374-L380

// Upon receipt of a Ping frame, an endpoint MUST send a Pong frame in
// response, unless it already received a Close frame. It SHOULD
// respond with Pong frame as soon as is practical. (RFC 6455)
if let Some(pong) = self.pong.take() {
  trace!("Sending pong reply");
  self.send_one_frame(stream, pong)?;
}

WIth this patch, all Autobahn tests from 1-8 pass. Fixed cases: 2.1, 2.2, 2.3, 2.4, 2.6, 2.9, 2.10, 2.11, 5.6, 5.7, 5.8, 5.19, 5.20

To run the test yourself, follow https://www.notion.so/denolandinc/Autobahn-WebSocket-testsuite-723a86f450ce4823b4ef9cb3dc4c7869?pvs=4

@littledivy littledivy enabled auto-merge (squash) February 13, 2023 14:27
@littledivy littledivy merged commit 9e3d433 into denoland:main Feb 13, 2023
littledivy added a commit that referenced this pull request Mar 15, 2023
This commit adds test for #17761
which was fixed by #17762. Verified
that test fails on Deno 1.30.1
kt3k pushed a commit that referenced this pull request Mar 16, 2023
This commit adds test for #17761
which was fixed by #17762. Verified
that test fails on Deno 1.30.1
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.

Extra pong sent by Deno's websocket implementation
2 participants