-
Notifications
You must be signed in to change notification settings - Fork 525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
websocket: avoid using Buffer.byteLength #3394
Conversation
lib/web/websocket/sender.js
Outdated
@@ -92,12 +89,12 @@ function createFrame (data, hint) { | |||
function toBuffer (data, hint) { | |||
switch (hint) { | |||
case sendHints.string: | |||
return Buffer.from(data) | |||
return typeof data === 'string' ? Buffer.from(data) : new Uint8Array(data.buffer, data.byteOffset, data.byteLength) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when can data not be a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR provides a buffer instead of a string, so it may not be a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it won't be a string anymore, shouldn't sendHints.string be removed and have this case default to sendHints.typedArray
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't do it because it's using sendHints.string
to determine if it's a text frame or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or delete toBuffer.
diff --git a/lib/web/websocket/constants.js b/lib/web/websocket/constants.js
index 2019b5b6..ac1c6fe1 100644
--- a/lib/web/websocket/constants.js
+++ b/lib/web/websocket/constants.js
@@ -48,9 +48,8 @@ const emptyBuffer = Buffer.allocUnsafe(0)
const sendHints = {
string: 1,
- typedArray: 2,
- arrayBuffer: 3,
- blob: 4
+ binary: 2,
+ blob: 3
}
module.exports = {
diff --git a/lib/web/websocket/sender.js b/lib/web/websocket/sender.js
index 130024f2..10f206be 100644
--- a/lib/web/websocket/sender.js
+++ b/lib/web/websocket/sender.js
@@ -51,7 +51,7 @@ class SendQueue {
const node = {
promise: item.arrayBuffer().then((ab) => {
node.promise = null
- node.frame = createFrame(ab, hint)
+ node.frame = createFrame(new Uint8Array(ab), sendHints.binary)
}),
callback: cb,
frame: null
@@ -83,19 +83,7 @@ class SendQueue {
}
function createFrame (data, hint) {
- return new WebsocketFrameSend(toBuffer(data, hint)).createFrame(hint === sendHints.string ? opcodes.TEXT : opcodes.BINARY)
-}
-
-function toBuffer (data, hint) {
- switch (hint) {
- case sendHints.string:
- return typeof data === 'string' ? Buffer.from(data) : data
- case sendHints.arrayBuffer:
- case sendHints.blob:
- return new Uint8Array(data)
- case sendHints.typedArray:
- return new Uint8Array(data.buffer, data.byteOffset, data.byteLength)
- }
+ return new WebsocketFrameSend(data).createFrame(hint === sendHints.string ? opcodes.TEXT : opcodes.BINARY)
}
module.exports = { SendQueue }
diff --git a/lib/web/websocket/websocket.js b/lib/web/websocket/websocket.js
index 9756c474..b664c2e3 100644
--- a/lib/web/websocket/websocket.js
+++ b/lib/web/websocket/websocket.js
@@ -253,10 +253,12 @@ class WebSocket extends EventTarget {
// increase the bufferedAmount attribute by the length of the
// ArrayBuffer in bytes.
- this.#bufferedAmount += data.byteLength
- this.#sendQueue.add(data, () => {
- this.#bufferedAmount -= data.byteLength
- }, sendHints.arrayBuffer)
+ const buffer = new Uint8Array(data)
+
+ this.#bufferedAmount += buffer.byteLength
+ this.#sendQueue.add(buffer, () => {
+ this.#bufferedAmount -= buffer.byteLength
+ }, sendHints.binary)
} else if (ArrayBuffer.isView(data)) {
// If the WebSocket connection is established, and the WebSocket
// closing handshake has not yet started, then the user agent must
@@ -270,10 +272,12 @@ class WebSocket extends EventTarget {
// not throw an exception must increase the bufferedAmount attribute
// by the length of data’s buffer in bytes.
- this.#bufferedAmount += data.byteLength
- this.#sendQueue.add(data, () => {
- this.#bufferedAmount -= data.byteLength
- }, sendHints.typedArray)
+ const buffer = new Uint8Array(data.buffer, data.byteOffset, data.byteLength)
+
+ this.#bufferedAmount += buffer.byteLength
+ this.#sendQueue.add(buffer, () => {
+ this.#bufferedAmount -= buffer.byteLength
+ }, sendHints.binary)
} else if (isBlobLike(data)) {
// If the WebSocket connection is established, and the WebSocket
// closing handshake has not yet started, then the user agent must
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't do it because it's using sendHints.string to determine if it's a text frame or not.
Seems a little obvious, surprised I forgot that.
Or delete toBuffer.
I thought about it for a little bit; toBuffer should stay. I don't think there's a simple way of explaining my reasoning but I'll try. When talking about the WebSocket spec we are referring to two different specs: RFC 6455 and the WHATWG websocket spec. The latter is an abstraction on top of the rfc. As such, if you take a look at the whatwg spec, you'll notice that it hardly does anything, instead preferring to reference the rfc for the logic. That was my approach when creating WebSocket, to implement RFC 6455 and build the WebSocket client on top of that. The easiest example I can give is WebsocketStream, which I was planning on using the rfc 6455 parts for, but not the whatwg parts.
(new paragraph, finally)
I think the queue system makes more sense now, you pass in a <data type> and it works. No need to convert the data beforehand (the conversions we do are part of the whatwg spec). You'll see this with every other part of WebSocket; it delegates the logic to a lower-level, rather than handling it inside the client directly.
Once we start moving the logic away from the RFC parts, it'll inevitably make it harder to implement WebsocketStream. Actually, I suspect that I'd end up reverting the changes done here to implement it if it was changed to remove toBuffer.
I will agree that I have not done a great job at separating whatwg logic from the protocol logic. I have some ideas which I may explore once we land this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... to get back to the original discussion, I have these ideas:
- rename
sendHints.string
tosendHints.text
since the type is no longer a string. - remove the
typeof data === 'string'
check becausedata
is no longer a string. If it can be a string, I think those sections should just wrap it in a Buffer.from(...) for now.
I appreciate the changes. I am more lenient with WebSocket changes because the whatwg spec doesn't do much and the rfc is for the protocol itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your reply gave me the following idea:
-
removewebidl.converters.WebSocketSendData
and add an asseertion function, e.g.,webidl.assetions.WebSocketSendData
-
simplify by moving websocket send data conversion to SendQueue and adding a single function e.g.
SendQueue#write(data[, callback ])
.
It also simplifies the implementation of #3395.
- Move close frame generation logic to a new function
9a60ec0
to
8fb8f0f
Compare
small faster