Websocket behaviour change since 1.44.3

irbull commented 1 month ago

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://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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

It seems like this is the PR that introduced the change in behaviour. https://github.com/denoland/deno/pull/24226. If I remove this, then the old behaviour returns.

irbull commented 1 month ago

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 commented 1 month ago

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

denoland / deno

Websocket behaviour change since 1.44.3 #24618