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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Array can make additional overhead... ok :) Speed is better than duplicates :)
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.
It should be benchmarked to see if it actually makes any difference :)
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.
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. |
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.
@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
bufferedAmount
getter take into account the data queued in the sender.Fixes #970.
cc: @alexions