feat(http): upgradeWebSocketStream

[korkje]

Closes https://github.com/denoland/deno/issues/14064 Added unstable upgradeWebSocketStream to enable server-side WebSocketStream usage. I took some inspiration from https://github.com/denoland/deno/pull/16732, which seemed to have gone stale and had some issues. Idle timeout (ping interval) didn't work as [_serverHandleIdleTimeout] was never called, and op calls were outdated.

[CLAassistant]

All committers have signed the CLA.

[korkje]

Just to explain my motivations in short;

From my understanding, the long-term goal is to return a WebSocketStream from upgradeWebSocket directly. How long-term we are talking here seems to be dependent upon when a WebSocketStream spec might be nailed down, which is highly uncertain.

Deno already provides an unstable WebSocketStream implementation, and so consistency is one of my motivations for providing this (also unstable) upgradeWebSocketStream function; If it is available for experimental testing on the client, why not on the server?

However, my main motivation is that how-/whenever the WebSocketStream interface might be properly spec'ed in the end, it could benefit from in-production experimental usage before-hand, so I think it would be a proper win-win to make it available under the --unstable flag as early as possible.

Going further, it might not be a crazy idea to keep both upgradeWebSocket and upgradeWebSocketStream as permanent alternatives, as users will always have differing preferences. Whether this is the way forward or not, may also be shed additional light on by making the upgradeWebSocketStream alternative available before rather than later.

[crowlKats]

I am overall in favour of this or #16732, however I think the other PR is better in terms of API, as it adds less changes to the overall API surface. This one is potentially more confusing for newer people, as there are 2 similar functions and not sure which they should use. But these are all personal nitpicks

[rikka0w0]

It makes more sense to add upgradeWebSocketStream as a separate function and keep the original upgradeWebSocket for compatibility

[korkje]

I just merged in main again, and updated the PR, would be great to see some progress on this 👍

[haochuan9421]

I think adding upgradeWebSocketStream while keeping upgradeWebSocket is a good solution. These won't cause confusion. Corresponds to WebSocketStream and WebSocket respectively! I really need this functionality as the current solution lacks backpressure. Why this PR not approved so long? @korkje @crowlKats

[haochuan9421]

The const { readable, writable } = await stream.connection; may need change to const { readable, writable } = await stream.opened;.

https://developer.chrome.com/docs/capabilities/web-apis/websocketstream

[korkje]

@haochuan9421 I agree with your sentiment that keeping upgradeWebSocketStream and upgradeWebSocket separate makes the most sense if avoiding confusion is the goal.

I don't know why, but in Deno's WebSocketStream implementation .connection actually used to be called .opened if I'm not mistaken. Anyway, that's more of a general WebSocketStream thing, this PR just deals with the server side part.

As for why the PR is on ice, I don't know, I assume it isn't a high priority, maybe because there hasn't been much movement with the WebSocketStream spec either, though I think it is odd to keep the Deno implementation as-is, with only client side support. Would love to hear more about what the goals are.

I'll be happy to pull changes (into the PR branch) and get everything up to date again if I get an indication that this can move forward.

[haochuan9421]

I agree that only keep the client side WebSocketStream is odd! Since WebSocketStream can be marked as "unstable", why not add an unstable upgradeWebSocketStream? It's important and urgent to have backpressure support for WebSocket in Deno. I can’t imagine that this PR has been submitted for almost one year and still hasn’t been processed. As for now, i can only use this kind of ugly workaroud and don't have idea about how to slow down the incoming side rate. if upgradeWebSocketStream not added to Deno, Do we need to start parsing websock from TCP byte by byte? Terrible!

async function socketReady(socket: WebSocket) {
  if (socket.bufferedAmount < 4194304) {
    return;
  }
  await new Promise((r) => setTimeout(r, 0));
  return socketReady(socket);
}

Deno.serve((req) => {
  if (req.headers.get("upgrade") != "websocket") {
    return new Response(null, { status: 501 });
  }
  const { socket, response } = Deno.upgradeWebSocket(req);
  socket.onopen = async () => {
    try {
      const file = await Deno.open('some_large_file');
      for await (const chunk of file.readable) {
        socket.send(chunk);
        await socketReady(socket);
      }
    } catch (error) {
      console.log("Error: ", error);
    } finally {
      socket.close();
    }
  };
  return response;
});

[haochuan9421]

@crowlKats I urgently request that this feature be reviewed and implemented in Deno as soon as possible!

[bartlomieju]

We've been checking in about the status of WebSocketStream spec and it's still not-close to being stabilized. I don't think we will land this PR until the spec stabilizes.

[rikka0w0]

We've been checking in about the status of WebSocketStream spec and it's still not-close to being stabilized. I don't think we will land this PR until the spec stabilizes.

Would it be possible to merge this PR and mark it as unstable?

[haochuan9421]

This feature should be valued，mark as unstable please!

[ry]

closing this because it's old