WebSocket Behaviour Alignment with Transport Layer #3
Comments
CMCDragonkai
changed the title
Websockets changes
|
@tegefaulkes I think you're also missing the backpressure problem. That should also be done here. |
|
Relevant commentary MatrixAI/Polykey#525 (comment) on websocket backpressure. Backpressure needs to be properly applied to the readable streams. I may need to make a new issue for this. This needs to be further specced out. |
|
https://chat.openai.com/share/8cb1acc0-88e9-4846-89f3-2bd3840dba6f
I'm working on the PKE reactivity system And I found that websocket transport actually means there's 3 layers of authentication possible.
In relation to our RPC system which is transport agnostic, it does make sense that our authentication at the RPC layer would have to be in the layer 3 payload protocol, because it's not aware of what the transport is. Same in QUIC where it's just a quic stream, but the quic connection may have done an MTLS earlier. It does mean that when you're doing the RPC authentication, the need to setup as a "basic token" format as per the discussion on the PR is sort of unnecessary. Since in that case we are simulating a http request header which is not required anymore. I think the case of using websockets as a transport layer, we would skip layer 2, because quic doesn't have as a common denominator, but there's always layer 1. Did you ensure that our current PK clients communicate over WSS to the PK agent server? |
|
Putting our client service auth into layer 3 means that the the transport layer already establishes a websocket connection from TCP to HTTP to Websockets. Note that websocket frames are always binary. If they fail authentication we need to close the stream gracefully. Regarding graceful websocket closing on the server side. I see a pattern like this (this is taken out of PKE's websocket handler): // A SocketStream is a Duplex stream with the `socket` object
wsHandler: (conn: SocketStream, request) => {
// This enables the websocket handler on
// `enterprise.polykey.com/`
// AND also on
// `org1.org2.enterprise.polykey.com/`.
// At some point we need to do authentication
// And this is just a demonstration for now
logger.info('Websocket Connection Received');
// If this is false, then we actually don't need to end auto
// But if it is true, we do need to!
logger.info(`Websocket Half Open ${conn.allowHalfOpen}`);
conn.on('drain', () => {
logger.info(`Resume websocket conn`);
conn.resume();
});
conn.on('data', (msg: Buffer) => {
console.log('INPUT DATA', JSON.stringify(msg.toString('utf-8')));
// If this is true, it is fully drained, otherwise backpressure!
const drained = conn.write(
JSON.stringify({
message: 'hello world'
}) + '\n'
);
logger.info(`Wrote to websocket conn, do we need to drain ${drained}, buffer size: ${conn.socket.bufferedAmount}`);
if (!drained) {
logger.info(`Pausing websocket conn`);
conn.pause();
}
});
conn.on('end', () => {
// This is only necessary if the socket has "allowHalfOpen"
logger.info('Websocket Connection Ended');
// We must end the connection!
conn.end();
});
}Notice that upon receiving end, we call end, this is because These situations we dealt with earlier when we were working on the |
|
Notice that on the server side, handling backpressure is quite easy. It's just a matter of writing, checking if the This is because this is just an echo handler, where the writable data is being generated in response to the readable data. If the writable data isn't being generated in response to readable data, then the backpressure has to push back all the way to whoever is calling the writing function. That's usually done just by providing a promise that the caller should be awaiting on. So the above example is a better demonstration of how to actually apply backpressure when reading from the websocket stream. However I believe you should have this already done when converting websocket connections to our web stream abstraction right? I haven't had the time to fully review your entire websocket transport implementation and of the corresponding quic integration yet, so we're just moving ahead to the 6th deployment of testnet and seeing if there are issues that pop up in production, but these things should have been detailed in the spec too during implementation. |
|
This may lead to factoring out both As per discussion in PKE. The In the future the underlying |
|
Right now neither Without this, right now all websocket connections require an independent TCP socket, an HTTP request and a HTTP 101 response, before getting a websocket connection. A WSS connection also requires TLS. This adds quite significant overhead to creating new websocket connections especially since all of our RPC transactions are one to one for websocket connections. Now it turns out that nodejs actually support the necessary protocol behaviour for HTTP2 to support this. The chrome browser also supports native websocket over http2. However firefox appears to have disabled it. But the main problem is This is unlikely to ever be done by However I found https://github.com/szmarczak/http2-wrapper/blob/master/examples/ws/index.js that appears to do it. It uses node's http2 module, and then wraps it in such a way that websockets will use HTTP2 streams. If this hack is sufficient, it would be possible to encapsulate this hack inside It's also possible that by the time For |
|
Full alignment is not possible until #2 is achieved. For now Instead this epic will focus only on what's left here. Further WS work will occur in js-ws. |
|
Since you removed |
|
It's a bit tricky to test since it's an internal implementation detail. But I did see back-pressure happening while testing. I can double check and attempt to make a test for it. |
|
A test should be made to check whether it applies on both ends. @amydevs can do this during the WS extraction. |
|
The backpressure has been revised, speced, and implemented in #5. Instead of calling |
|
Done with #5. |
Specification
This issue is an epic for tracking the pending work for the
Websocketscode.Specifically an extraction of the behaviour to
js-wswhich behave similarly tojs-quic.Additional context
TLSconfigforWebSocketServerPolykey#511uwswithwsPolykey#540Tasks
WebSocketStreamexists and we can mux and demux on top of a singleWebSocketConnectionWebSocketConectionequivalent toQUICConnection, but as a 1 to 1 toWebSocketThe text was updated successfully, but these errors were encountered: