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.

Sign up for GitHub

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

Jump to bottom

Websocket behaviour change since 1.44.3 #24618

Closed
irbull opened this issue Jul 17, 2024 · 5 comments
Closed

Websocket behaviour change since 1.44.3 #24618

irbull opened this issue Jul 17, 2024 · 5 comments

Comments

@irbull
Copy link
Contributor

irbull commented Jul 17, 2024

Version: Deno 1.44.3+

Since Deno 1.44.3 and later the following web socket example no longer runs:

const site_html = `
<html>\n
<script>\n
const ws = new WebSocket("ws:https://localhost:8080/ws");\n
ws.onmessage = (event) => document.body.innerHTML = event.data;\n
</script>\n
</html>
`;

async function handleRequest(req: Request): Promise<Response> {
  const url = new URL(req.url);
  if (url.pathname.startsWith("/ws")) {
    const { socket, response } = Deno.upgradeWebSocket(req);
    const deferred = Promise.withResolvers<void>();
    socket.onopen = () => {
      console.log("WebSocket connection opened");
      deferred.resolve();
    };
    await deferred.promise;
    socket.send("Hello, world from Websocket!");
    return response;
  }
  // Just return the HTML for the site
  return new Response(site_html, {
    status: 200,
    headers: { "content-type": "text/html" },
  });
}

Deno.serve({ port: 8080 }, (req) => handleRequest(req));

Seems to be deadlock, in that the response is not returned until the websocket is opened, but the web socket is no longer opened until the Response is returned.

I can do some digging to find out what commit introduced this change.
@littledivy
Copy link
Member

Seems intended behaviour.

const deferred = Promise.withResolvers<void>();
socket.onopen = () => {
   console.log("WebSocket connection opened");
   deferred.resolve();
};
await deferred.promise;

onopen is not called until a response is returned to complete the websocket handshake.

@irbull
Copy link
Contributor Author

irbull commented Jul 17, 2024

@littledivy I agree with you, it was only that this did work in 1.44.2 (and maybe earlier). I've fixed our code to return the response and properly handle the websocket in the response to onopen. @bartlomieju asked me to file an issue. It wasn't clear to me what the http spec says here.

At least we can document this here in case others hit this too.

I'll try to dig up the exact commit that caused this so we can document that too.

@irbull
Copy link
Contributor Author

irbull commented Jul 17, 2024

It seems like this is the PR that introduced the change in behaviour. #24226. If I remove this, then the old behaviour returns.

@irbull
Copy link
Contributor Author

irbull commented Jul 17, 2024

The documentation states:

The original request must be responded to
   * with the returned response for the websocket upgrade to be successful.

Which is probably good enough to explain the issue. I'm happy to close this as I agree that this code doesn't conform to the documented API. Since this did work in previous versions of Deno but failed after 1.44.3, this issue may help another person who stumbles upon this.

@lucacasonato
Copy link
Member

I'll close - if others run into this they can find this issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@irbull @lucacasonato @littledivy