Take into account the data queued in the sender#971
Conversation
This makes the `bufferedAmount` getter take into account the data queued in the sender.
| readOnly = false; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd move this block of code to a separate function, like
makeBuffer (data) {
var readOnly = true;
if (data && !Buffer.isBuffer(data)) {
if (data instanceof ArrayBuffer) {
data = Buffer.from(data);
} else if (ArrayBuffer.isView(data)) {
data = viewToBuffer(data);
} else {
data = Buffer.from(data);
readOnly = false;
}
}
return [data, readOnly];
}
Usage:
[data, readOnly] = makeBuffer(data);
There was a problem hiding this comment.
I also don't like having this code duplication. The reason for not using a separate function is to avoid creating an array.
There was a problem hiding this comment.
Agree. Array can make additional overhead... ok :) Speed is better than duplicates :)
There was a problem hiding this comment.
It should be benchmarked to see if it actually makes any difference :)
There was a problem hiding this comment.
Another option would be to add the readOnly flag to the returned buffer. For example:
const kReadOnly = Symbol('read-only');
function toBuffer(data) {
var readOnly = true;
if (!Buffer.isBuffer(data)) {
if (data instanceof ArrayBuffer) {
data = Buffer.from(data);
} else if (ArrayBuffer.isView(data)) {
data = viewToBuffer(data);
} else {
data = Buffer.from(data);
readOnly = false;
}
}
data[kReadOnly] = readOnly;
return data;
}but I'm still not happy with it as that changes the buffer hidden class.
|
I would like to have at least one LGTM from the other @websockets/admin members before merging. |
|
@alexions thanks but I'm -1 on that approach as that proposes to check the things twice (2x |
|
ok. Agree that your changes will work faster. |
|
Before this change the whole |
|
I'll merge this tomorrow if there are no objections. |
|
@alexions thanks! |
|
@lpinca you're welcome! |
This makes the
bufferedAmountgetter take into account the data queued in the sender.Fixes #970.
cc: @alexions