denoland / fastwebsockets

A fast RFC6455 WebSocket implementation
https://docs.rs/fastwebsockets/
Apache License 2.0
880 stars 66 forks source link

feat: Close tcp after sending a close frame #72

Open jespertheend opened 8 months ago

jespertheend commented 8 months ago

Fixes https://github.com/denoland/deno/issues/21642

jespertheend commented 8 months ago

I wasn't able to build fastwebsockets on my macbook, so I couldn't run the test suite. But I was able to build deno with these changes and I tested this manually using this script:

script

Run with `deno run -A websocket.js` ```javascript Deno.serve({ hostname: "0.0.0.0", port: 8000 }, (request) => { const url = new URL(request.url); if (url.pathname == "/") { return new Response( ` `, { headers: { "Content-Type": "text/html", }, }, ); } else if (url.pathname == "/ws") { const { socket, response } = Deno.upgradeWebSocket(request); socket.addEventListener("open", () => { console.log("open"); }); socket.addEventListener("message", () => { console.log("close"); socket.close(3123, "custom reason"); }); return response; } return new Response("not found", { status: 404 }); }); ```

I also tried this out on two moderately sized projects that make heavy use of websockets, both seemed to run without any issues.

Let me know if this is the right approach for this. One issue I can see is that the read_half isn't being closed, but I'm not quite sure how to go about that.

littledivy commented 8 months ago

@jespertheend one of the tests is failing: https://github.com/denoland/fastwebsockets/actions/runs/8209255112/job/22459970144?pr=72#step:6:455

chefnaphtha commented 5 months ago

i hate to be that guy but this is pretty important, error handling on chromium is broken until this gets merged. looks like github purged the test logs, can someone re-run them?

haochuan9421 commented 5 months ago
image

Using tcpdump and WireShark to capture the packages, when socket.close() is called in Deno, it seems that it send close message twice!

jespertheend commented 5 months ago

Last time I checked, this PR fixes the issue, but iirc I couldn't get the tests to pass and I haven't had time to figure out why yet.