feat(websocket): expand diagnostics channel payloads#4888
feat(websocket): expand diagnostics channel payloads#4888islandryu wants to merge 3 commits intonodejs:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4888 +/- ##
==========================================
- Coverage 92.89% 92.79% -0.11%
==========================================
Files 112 112
Lines 35665 35774 +109
==========================================
+ Hits 33131 33195 +64
- Misses 2534 2579 +45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lib/web/websocket/sender.js
Outdated
|
|
||
| for (let i = 0; i < payloadData.length; ++i) { | ||
| payloadData[i] = frame[payloadOffset + i] ^ maskKey[i & 3] | ||
| } |
There was a problem hiding this comment.
diagnostics channels are meant to be low overhead. This is the opposite of that
There was a problem hiding this comment.
Changed to use the data before masking.
KhafraDev
left a comment
KhafraDev
left a comment
There was a problem hiding this comment.
I don't like the changes. Diagnostics channels should be unobtrusive.
lib/web/websocket/sender.js
Outdated
| this.#socket.uncork() | ||
| } else { | ||
| // direct writing | ||
| publishFrame(this.#websocket, hint === sendHints.text ? opcodes.TEXT : opcodes.BINARY, true, toBuffer(item, hint)) |
There was a problem hiding this comment.
the toBuffer will make WebSocket slower for the majority of people not using diagnostics channels
There was a problem hiding this comment.
Moved the toBuffer call after the hasSubscribers check.
lib/web/websocket/sender.js
Outdated
| const node = { | ||
| promise: item.arrayBuffer().then((ab) => { | ||
| node.promise = null | ||
| publishFrame(this.#websocket, opcodes.BINARY, true, new Uint8Array(ab)) |
|
@mcollina @KhafraDev |
| const rsv3 = buffer[0] & 0x10 | ||
|
|
||
| if (!isValidOpcode(opcode)) { | ||
| this.publishFrameError(new Error('Invalid opcode received')) |
There was a problem hiding this comment.
Isn't there a cleaner way to handle this? Repeating these same lines before every failWebsocketConnection call seems like it could lead to maintenance issues.
| channels.socketError.publish(err) | ||
| channels.socketError.publish({ | ||
| error: err, | ||
| websocket: undefined | ||
| }) |
There was a problem hiding this comment.
What was the reason for this change? Wouldn't this be considered a breaking change?
| this.#socket.uncork() | ||
| } else { | ||
| // direct writing | ||
| publishFrame(this.#websocket, hint === sendHints.text ? opcodes.TEXT : opcodes.BINARY, true, item, hint) |
There was a problem hiding this comment.
| publishFrame(this.#websocket, hint === sendHints.text ? opcodes.TEXT : opcodes.BINARY, true, item, hint) | |
| publishFrame(this.#websocket, opcodes.BINARY, true, item, hint) |
unreachable branch
| publishFrame(this.#websocket, opcodes.BINARY, true, ab, hint) | ||
| node.diagnosticInfo.published = true |
There was a problem hiding this comment.
Why is it published when resolved? Wouldn't that be called before it's actually sent? Also, wouldn't that mess up the order?
| channels.frameSent.publish({ | ||
| websocket, | ||
| opcode, | ||
| mask, |
There was a problem hiding this comment.
Since this is client-side only, masked is always true. Is this really necessary?
| channels.frameReceived.publish({ | ||
| websocket: this.#handler.websocket, | ||
| opcode: this.#info.opcode, | ||
| mask: this.#info.masked, |
There was a problem hiding this comment.
Since this is client-side only, masked is always false. Is this really necessary?
5 participants
This relates to...
Fixes: #4673
Rationale
I would like to extend the WebSocket events in order to send CDP data.
Changes
Introduces undici:websocket:created, undici:websocket:handshakeRequest, frameSent, frameReceived, and frameError coverage.
Features
Bug Fixes
N/A
Breaking Changes and Deprecations
Status