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.

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

napi: implement napi_is_detached_arraybuffer #30613

Closed

Conversation

lundibundi
Copy link
Member

This implements ArrayBuffer#IsDetachedBuffer operation as per ECMAScript
specification Section 24.1.1.2 https://tc39.es/ecma262/#sec-isdetachedbuffer.

Closes: #29955

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Also superseeds #30317.

This implements ArrayBuffer#IsDetachedBuffer operation as per ECMAScript
specification Section 24.1.1.2 https://tc39.es/ecma262/#sec-isdetachedbuffer

Closes: nodejs#29955
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 23, 2019
@lundibundi lundibundi added the node-api Issues and PRs related to the Node-API. label Nov 23, 2019
@nodejs-github-bot
Copy link
Collaborator

The `ArrayBuffer` is considered detached if its internal data is `null`.

This API represents the invocation of the `ArrayBuffer` `IsDetachedBuffer`
operation as defined in [Section 24.1.1.2][] of the ECMAScript Language
Copy link
Member

Choose a reason for hiding this comment

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

section numbers tend to change, i'd just say "as defined in the ecmascript language specifiation."

Copy link
Member Author

@lundibundi lundibundi Nov 24, 2019

Choose a reason for hiding this comment

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

This looks like a good idea but a lot of the other functions (in this doc) have links to the appropriate sections in the spec so I'm not sure.

@lundibundi
Copy link
Member Author

Wow, another ICE here

https://ci.nodejs.org/job/node-test-commit-arm/nodes=debian9-docker-armv7/27580/console
debian9-docker-armv7

23:37:07 ../src/api/exceptions.cc: In function 'v8::Local<v8::Value> node::UVException(v8::Isolate*, int, const char*, const char*, const char*, const char*)':
23:37:07 ../src/api/exceptions.cc:146:53: internal compiler error: Segmentation fault
23:37:07    e->Set(env->context(), env->code_string(), js_code).Check();
23:37:07                                                      ^
23:37:07 Please submit a full bug report,
23:37:07 with preprocessed source if appropriate.
23:37:07 See <file:///usr/share/doc/gcc-6/README.Bugs> for instructions.
23:37:07 libnode.target.mk:330: recipe for target '/home/iojs/build/workspace/node-test-commit-arm/nodes/debian9-docker-armv7/out/Release/obj.target/libnode/src/api/exceptions.o' failed
23:37:07 make[2]: *** [/home/iojs/build/workspace/node-test-commit-arm/nodes/debian9-docker-armv7/out/Release/obj.target/libnode/src/api/exceptions.o] Error 1

/cc @addaleax

@nodejs-github-bot
Copy link
Collaborator

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 24, 2019
@legendecas
Copy link
Member

Just found some nits in tests... 😅

@lundibundi lundibundi removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 24, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lundibundi
Copy link
Member Author

Well, the second ICE on the same machine

https://ci.nodejs.org/job/node-test-commit-arm/27606/nodes=debian9-docker-armv7/console
debian9-docker-armv7

17:38:16 ../src/node_v8.cc: In function 'void node::Initialize(v8::Local<v8::Object>, v8::Local<v8::Value>, v8::Local<v8::Context>, void*)':
17:38:16 ../src/node_v8.cc:175:57: internal compiler error: Segmentation fault
17:38:16                Uint32::NewFromUnsigned(env->isolate(), i)).Check();
17:38:16                                                          ^
17:38:16 ../src/node_v8.cc:51:3: note: in expansion of macro 'V'
17:38:16    V(2, total_physical_size, kTotalPhysicalSizeIndex)                          \
17:38:16    ^
17:38:16 ../src/node_v8.cc:177:3: note: in expansion of macro 'HEAP_STATISTICS_PROPERTIES'
17:38:16    HEAP_STATISTICS_PROPERTIES(V)
17:38:16    ^~~~~~~~~~~~~~~~~~~~~~~~~~
17:38:16 Please submit a full bug report,
17:38:16 with preprocessed source if appropriate.
17:38:16 See <file:///usr/share/doc/gcc-6/README.Bugs> for instructions.
17:38:16 libnode.target.mk:330: recipe for target '/home/iojs/build/workspace/node-test-commit-arm/nodes/debian9-docker-armv7/out/Release/obj.target/libnode/src/node_v8.o' failed
17:38:16 make[2]: *** [/home/iojs/build/workspace/node-test-commit-arm/nodes/debian9-docker-armv7/out/Release/obj.target/libnode/src/node_v8.o] Error 1
17:38:16 make[2]: *** Waiting for unfinished jobs....

@nodejs-github-bot
Copy link
Collaborator

@lundibundi
Copy link
Member Author

Hm, ICE on debian9-docker-armv7 once again, not sure how to fix that.

22:57:02 ../src/node_v8.cc: In function 'void node::Initialize(v8::Local<v8::Object>, v8::Local<v8::Value>, v8::Local<v8::Context>, void*)':
22:57:02 ../src/node_v8.cc:169:59: internal compiler error: Segmentation fault
22:57:02                                 env->heap_statistics_buffer(),
22:57:02                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
22:57:02 Please submit a full bug report,
22:57:02 with preprocessed source if appropriate.
22:57:02 See <file:///usr/share/doc/gcc-6/README.Bugs> for instructions.
22:57:02 libnode.target.mk:330: recipe for target '/home/iojs/build/workspace/node-test-commit-arm/nodes/debian9-docker-armv7/out/Release/obj.target/libnode/src/node_v8.o' failed
22:57:02 make[2]: *** [/home/iojs/build/workspace/node-test-commit-arm/nodes/debian9-docker-armv7/out/Release/obj.target/libnode/src/node_v8.o] Error 1

/cc @nodejs/n-api @addaleax perhaps you can help?

@addaleax
Copy link
Member

@lundibundi Somewhere @bnoordhuis suggested that those ICEs might be the result of an out-of-memory situation … I think it’s a machine that doesn’t have a ton of memory, so that might make sense?

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM once my suggestion addition of the stability is added.

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 28, 2019
@targos
Copy link
Member

targos commented Jan 14, 2020

This needs a backport to land on v12.x-staging because V8 7.8 doesn't have ArrayBuffer::GetBackingStore

lundibundi added a commit to lundibundi/node that referenced this pull request Jan 20, 2020
This implements ArrayBuffer#IsDetachedBuffer operation as per ECMAScript
specification Section 24.1.1.2 https://tc39.es/ecma262/#sec-isdetachedbuffer

Closes: nodejs#29955

Backport-PR-URL: nodejs#31422
PR-URL: nodejs#30613
Fixes: nodejs#29955
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
tniessen added a commit to tniessen/node-addon-api that referenced this pull request Jan 26, 2020
tniessen added a commit to tniessen/node-addon-api that referenced this pull request Jan 26, 2020
MylesBorins pushed a commit that referenced this pull request Jan 30, 2020
This implements ArrayBuffer#IsDetachedBuffer operation as per ECMAScript
specification Section 24.1.1.2 https://tc39.es/ecma262/#sec-isdetachedbuffer

Closes: #29955

Backport-PR-URL: #31422
PR-URL: #30613
Fixes: #29955
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This implements ArrayBuffer#IsDetachedBuffer operation as per ECMAScript
specification Section 24.1.1.2 https://tc39.es/ecma262/#sec-isdetachedbuffer

Closes: #29955

Backport-PR-URL: #31422
PR-URL: #30613
Fixes: #29955
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
legendecas pushed a commit to legendecas/node that referenced this pull request Jul 1, 2020
This implements ArrayBuffer#IsDetachedBuffer operation as per ECMAScript
specification Section 24.1.1.2 https://tc39.es/ecma262/#sec-isdetachedbuffer

Closes: nodejs#29955

PR-URL: nodejs#30613
Fixes: nodejs#29955
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 1, 2020
This implements ArrayBuffer#IsDetachedBuffer operation as per ECMAScript
specification Section 24.1.1.2 https://tc39.es/ecma262/#sec-isdetachedbuffer

Closes: #29955

PR-URL: #30613
Backport-PR-URL: #33061
Fixes: #29955
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@richardlau richardlau mentioned this pull request Jul 2, 2020
4 tasks
tniessen added a commit to tniessen/node-addon-api that referenced this pull request Nov 5, 2020
mhdawson pushed a commit to nodejs/node-addon-api that referenced this pull request Nov 5, 2020
Superlokkus pushed a commit to Superlokkus/node-addon-api that referenced this pull request Nov 20, 2020
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

n-api: add napi_is_detached_arraybuffer()
9 participants