deno icon indicating copy to clipboard operation
deno copied to clipboard

upgradeWebSocket could return a WebSocketStream

Open MarkTiedemann opened this issue 2 years ago • 7 comments

Currently, Deno.upgradeWebSocket returns a WebSocket. It could return a WebSocketStream, too, for example:

const conn = await Deno.listen({ port: 80 });
const httpConn = Deno.serveHttp(await conn.accept());
const e = await httpConn.nextRequest();
if (e) {
  const { stream, response } = Deno.upgradeWebSocket(e.request);
  const [_, { writable, readable }] = await Promise.all([
    e.respondWith(response),
    stream.connection,
  ]);
  // Use writable and/or readable
}

MarkTiedemann avatar Mar 21 '22 19:03 MarkTiedemann

Yes, this is the plan longterm, and was actually the plan originally to some degree (for both statements, its rather not alongside, but instead of), however, the idea is that the server side and client side use the same API for a smoother experience, but since browsers do not have WebSocketStream yet, this isnt exactly the case

crowlKats avatar Mar 21 '22 19:03 crowlKats

I'm looking forward to this feature as well 😃 I have Deno programs as both the client and the server, so I get to do the nice W3C streams stuff on the client side, just to deal with the classic WebSocket callbacks on the server side.

danopia avatar May 20 '23 10:05 danopia

I discussed it with @crowlKats recently and the fact that there's still no spec makes it quite hard to ship in Deno.

@crowlKats anything changed since we spoke about it?

bartlomieju avatar May 22 '23 13:05 bartlomieju

Sadly no, there has been no progress from what I am aware

crowlKats avatar May 23 '23 17:05 crowlKats

Could it make sense to, at least while WebSocketStream is getting all spec'ed and ready, provide an experimental/unstable upgradeWebSocketStream alternative alongside upgradeWebSocket? Would prevent any breaking changes for users, while also letting users choose their preferred API.

korkje avatar Aug 24 '23 13:08 korkje

Tried my hand at a PR taking some inspiration from, and trying to improve upon, the earlier PR (https://github.com/denoland/deno/pull/16732) by @crowlKats.

korkje avatar Aug 25 '23 17:08 korkje

export class WebSocketStream {
    public socket: WebSocket;
    public readable: ReadableStream<Uint8Array>;
    public writable: WritableStream<Uint8Array>;

    constructor(socket: WebSocket) {
        this.socket = socket;
        this.readable = new ReadableStream({
            start(controller) {
                socket.onmessage = function ({ data }) {
                    // console.log(data);
                    return controller.enqueue(new Uint8Array(data));
                };
                socket.onerror = (e) => controller.error(e);
                socket.onclose = (/* e */) => controller.close(/* e */);
            },
            cancel(/* reason */) {
                socket.close();
            },
        });
        this.writable = new WritableStream({
            start(controller) {
                socket.onerror = (e) => controller.error(e);
            },
            write(chunk) {
                // console.log(chunk);
                socket.send(chunk);
            },
            close() {
                socket.close();
            },
            abort(e) {
                console.error(e);
                socket.close();
            },
        });
    }
}

masx200 avatar Jan 29 '24 04:01 masx200

export class WebSocketStream {

One important feature of the WebSocketStream is the backpressure handling, and this simple wrapper does not support backpressure at all. It is possible to implement backpressure on the outgoing stream via WebSocket: bufferedAmount, but not possible on the incoming stream. Missing the backpressure support can overwhelm the receiver and potentially cause out-of-memory.

rikka0w0 avatar Feb 29 '24 15:02 rikka0w0