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

feat: Close tcp after sending a close frame #72

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jespertheend
Copy link

@jespertheend jespertheend commented Mar 8, 2024

@jespertheend
Copy link
Author

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

Deno.serve({ hostname: "0.0.0.0", port: 8000 }, (request) => {
  const url = new URL(request.url);
  if (url.pathname == "/") {
    return new Response(
      `
      <script>
        const url = new URL(location.href);
        url.pathname = "/ws";
        url.protocol = "ws:";
        const ws = new WebSocket(url.href);
        ws.onclose = (e) => {
          console.log("closed", e);
        }
      </script>
      <button onclick="ws.send('close')">request close</button>
      <button onclick="ws.close()">close</button>
    `,
      {
        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
Copy link
Member

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

@chefnaphtha
Copy link

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
Copy link

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
Copy link
Author

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.

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
4 participants