Skip to content
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.

Sign up for GitHub

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

Jump to bottom

util: add isArrayBufferDetached method #45512

Closed
wants to merge 3 commits into from
Closed

util: add isArrayBufferDetached method #45512

wants to merge 3 commits into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Nov 18, 2022 •
edited
Loading

CC @nodejs/streams @nodejs/util @nodejs/performance

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. typings web streams labels Nov 18, 2022
@anonrig
Copy link
Member Author

anonrig commented Nov 18, 2022

CC @daeyeon, since he was the original author of this TODO.

@anonrig anonrig force-pushed the feat/array-buffer-detached branch 2 times, most recently from 9f4da15 to 55484ba Compare November 18, 2022 23:40
@anonrig anonrig mentioned this pull request Nov 18, 2022
Closed
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48014/

addaleax
addaleax reviewed Nov 19, 2022
View reviewed changes
doc/api/util.md Outdated Show resolved Hide resolved
@tniessen
Copy link
Member

b) Should this really be a public API?

Adding to @addaleax's point, if there is justification for such a public API in Node.js, isn't there justification for such an API in any JavaScript runtime, in which case it would be better to find a compatible approach across runtimes?

@anonrig
Copy link
Member Author

anonrig commented Nov 19, 2022

b) Should this really be a public API?

Adding to @addaleax's point, if there is justification for such a public API in Node.js, isn't there justification for such an API in any JavaScript runtime, in which case it would be better to find a compatible approach across runtimes?

I'm +0 on the public API. But I don't think there is a better & compatible approach across runtimes (for now), since WasDetached is not currently supported at Deno (using V8 10.9.194.1)

@anonrig anonrig force-pushed the feat/array-buffer-detached branch 3 times, most recently from 255977c to 81b13d1 Compare November 19, 2022 18:14
jasnell
jasnell reviewed Nov 19, 2022
View reviewed changes
doc/api/util.md Outdated Show resolved Hide resolved
doc/api/util.md Show resolved Hide resolved
lib/internal/webstreams/util.js Show resolved Hide resolved
jasnell
jasnell reviewed Nov 19, 2022
View reviewed changes
doc/api/util.md Outdated Show resolved Hide resolved
@anonrig anonrig requested a review from jasnell November 19, 2022 20:02
aduh95
aduh95 reviewed Nov 19, 2022
View reviewed changes
lib/internal/util/types.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Nov 19, 2022

If a universal solution is desired, and there's compelling use cases for it, a JS language proposal seems appropriate.

@anonrig anonrig mentioned this pull request Nov 19, 2022
Merged
@github-actions github-actions bot mentioned this pull request Nov 20, 2022
Open
14 tasks
@mcollina
Copy link
Member

What's the use case for this? The PR description is empty.

LiviaMedeiros
LiviaMedeiros reviewed Nov 20, 2022
View reviewed changes
doc/api/util.md Outdated Show resolved Hide resolved
doc/api/util.md Show resolved Hide resolved
lib/internal/util/types.js Outdated
@@ -54,8 +57,16 @@ function isBigUint64Array(value) {
return TypedArrayPrototypeGetSymbolToStringTag(value) === 'BigUint64Array';
}

function isArrayBufferDetached(value) {
if (ArrayBufferPrototypeGetByteLength(value) === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do this before checking if value is an instance of ArrayBuffer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user passes something that's not an ArrayBuffer, instead of returning false the function would throw a TypeError, which is surprising. We should at least document this behavior if we want to keep it (another option is to not expose it publicly and keep it internal, or expose a slower version that first checks if the parameter is an ArrayBuffer before calling the internal util).

@jasnell
Copy link
Member

jasnell commented Nov 20, 2022

@mcollina:

What's the use case for this? The PR description is empty.

There are a few cases (e.g. in web streams) where we are required to check if the ArrayBuffer has been detached. Currently we only approximate the check by seeing if it is a zero-length ArrayBuffer, but that's really not quite accurate.

@mcollina
Copy link
Member

If we need it only for our webstreams API, it would be better to:

  1. only expose it internally
  2. include those usages in the PR

@github-actions github-actions bot mentioned this pull request Nov 21, 2022
Open
15 tasks
@LiviaMedeiros
Copy link
Contributor

If performance cost of checking if ArrayBuffer is detached will be negligible, it would make sense to widely use it in other parts of Node.js API.
In most cases, passing a view with detached buffer is meaningless and implies an error in userland code. Currently, the outcome depends on underlying implementation:

  • If it tries to use the buffer (e.g. make a new view with new Uint8Array(passedData.buffer), or a copy with passedData.slice()), the TypeError will be thrown.
  • Otherwise it's silently interpreted as zero-size buffer; for example, fs.writeFileSync(filePath, dataBuffer) will truncate file to zero if dataBuffer.buffer happens to be detached.

@anonrig anonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Nov 21, 2022
@anonrig
Copy link
Member Author

anonrig commented Nov 21, 2022

I'm working with @ljharb on making a proposal to TC39 for detach and isDetached. Until, it is released, I wanted to give the option to both functionality to detach and detect isDetached to the userland. This is one of the first pull request, when/if merged, will be continued with detachArrayBuffer() functionality. Some of the reviews on this pull request was around why we should expose isDetached to the userland.

This particular question is better answered with the original ArrayBuffer.prototype.transfer() proposal.

aduh95
aduh95 reviewed Nov 21, 2022
View reviewed changes
lib/internal/util/types.js
@@ -54,8 +58,16 @@ function isBigUint64Array(value) {
return TypedArrayPrototypeGetSymbolToStringTag(value) === 'BigUint64Array';
}

function isArrayBufferDetached(value) {
if (value instanceof ArrayBuffer && ArrayBufferPrototypeGetByteLength(value) === 0) {
Copy link
Contributor

@aduh95 aduh95 Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't be surprised if that instanceof check had a dramatic impact on performance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referencing: https://github.com/RafaelGSS/nodejs-bench-operations/blob/main/RESULTS-v19.md#comparison-using-instanceof

I can change it to value != null && ... , and expect .byteLength to be the differentiator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best course of action is to make an internal isArrayBufferDetached without any instanceof check, and if we have a public one, it would return value instanceof ArrayBuffer && internal.isArrayBufferDetached(value).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aduh95 #45568

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, instanceof is brittle and can be forged, and doesn’t work cross-realm. In JS the way to check this is using the byteLength getter in a try/catch.

@anonrig anonrig mentioned this pull request Nov 21, 2022
Merged
This was referenced Nov 22, 2022
Open
Open
@anonrig
Copy link
Member Author

anonrig commented Dec 3, 2022

I'm closing this pull request in favor of Array.prototype.detached proposal, which is currently in Stage 2.

@anonrig anonrig closed this Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LiviaMedeiros LiviaMedeiros LiviaMedeiros left review comments

@aduh95 aduh95 aduh95 left review comments

@daeyeon daeyeon daeyeon left review comments

@addaleax addaleax addaleax left review comments

@ljharb ljharb ljharb left review comments

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. typings web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@anonrig @nodejs-github-bot @tniessen @ljharb @mcollina @jasnell @LiviaMedeiros @addaleax @daeyeon @aduh95