WebSockets

[KhafraDev]

permessage-deflate support

• Is this something we need/want to support? Servers that are setup correctly should work even if the client doesn't support it.
• Memory fragmentation issues when using zlib?
• Added complexity of decompressing frames asynchronously

---

Performance

• Consume the least amount of bytes possible, rather than concatenating every chunk available. See ws' implementation. Fixed in 5165d67
• Switch to Buffer.allocUnsafe in lib/websocket/frame.js Fixed in b6844f0
• Handle TODO comments labeled as "optimize this".
• Benchmarks
• Use FastBuffer (Buffer[Symbol.species])

---

Tests

• Add ws' test suite. Note this isn't easy because ws' api does not strictly follow the spec and it occasionally uses internal, underscored properties. Also note that a lot of the validation tests are already handled by the WPTs.
• Autobahn testsuite
• 100% code coverage for lib/websockets Code coverage is high enough; WPTs are not counted, which make up a majority of the tests.
• Test more strange/error conditions:
  • Chunks that contain thousands of frames
  • Chunks that receive a pong/close frame in the middle of a fragmented message (Control frames are already handled the same.)
  • Sending invalid frames

---

Bugs

• using WebSocket.send with a Blob asynchronously writes the blob data to the socket. This can cause issues when concurrently sending a blob with anything else. Note: we need support in node core to read a Blob synchronously.
• ByteParser.run runs recursively, meaning the max call stack can be exceeded under certain conditions (ie. receiving thousands of frames in a single chunk). Fixed in 1b858fb

---

Features

• Setting an undici Dispatcher rather than using the global dispatcher by default.
• Letting the client generate the mask for performance reasons.

---

WebSocketStream

• https://github.com/ricea/websocketstream-explainer
• No real spec, the explainer is superficial.
• API is much better; allows for expansion (ie. second param is a dictionary rather than a list/string).
• Probably much slower
• requires rewrite of WebSocket internals

[jimmywarting]

Note: we need support in node core to read a Blob synchronously.

Hmm, would that be really be any good?
if we ever get something like blob backed up by the fs and you would be able to create them from a network mounted cloud drive. then that would block quite a bit.

[KhafraDev]

would that be really be any good?

No, but it's the only way that issue could be fixed without adding complexity.

[KhafraDev]

Undici only implements a websocket client, the spec one that has documentation on mdn. If you want to use the lower level dispatcher api from undici, you can use the onUpgrade callback.

[mcollina]

Implementing a server is definitely out of scope: use ws - none of the spec-compliant work has the settings or APIs needed to implement a robust WebSocket server. On another note, we should likely document this somewhere, as this question would come up quite often.

[jimmywarting]

none of the spec-compliant work has the settings or APIs needed to implement a robust WebSocket server

What would it take to maybe say: Build a own npm package that depends on undici WebSocket and tries to build a compatible server? would it be even at all possible by using import xyz from 'undici/lib/websocket/*'? and would it require a lot of work?

I'm guessing it would have to work quite differently if you used http/1/2/3 quick WebTransport or whatever is the new "thing"

we should likely document this somewhere, as this question would come up quite often.

I bet so too. Guess ppl are going to ask about this when they learn that there is a built in WebSocket later

[yume-chan]

Is there any update on the "Setting an undici Dispatcher rather than using the global dispatcher by default." feature?

I need to send each WebSocket to different targets without modifying the Host header (the server checks it). By creating multiple dispatchers I can use the Dispatcher#connect method to override the target. But now I have to use this hack:

const agent = new Agent({
    factory(origin, opts) {
        const pool = new Pool(origin, {
            ...opts,
            factory(origin, opts) {
                const client = new Client(origin, opts);
                // Remote debugging validates `Host` header to defend against DNS rebinding attacks.
                // But we can only pass socket name using hostname, so we need to override it.
                (client as any)[Symbols.kHostHeader] = "Host: localhost\r\n";
                return client;
            },
        });
        return pool;
    },
    async connect(options, callback) {
        try {
            const socket = await device.createSocket(
                "localabstract:" + options.hostname
            );
            callback(null, new AdbUndiciSocket(socket) as unknown as Socket);
        } catch (e) {
            callback(e as Error, null);
        }
    },
});
// WebSocket only uses global dispatcher
setGlobalDispatcher(agent);

I'm also concerned that undici used in other libraries will also be affected by my global dispatcher.

[KhafraDev]

Is there any updates on the "Setting an undici Dispatcher rather than using the global dispatcher by default." feature?

Yes, I'm planning on implementing my proposal which will allow us to do so. I should have a PR in the next day, if I remember.