n-api: add `napi_detach_arraybuffer` #29768

legendecas · 2019-09-30T01:44:19Z

As ArrayBuffer#detach is an ecma spec operation (Section 24.1.1.3), it might be good to have it in N-API.

Fixes #29674

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

doc/api/n-api.md

src/js_native_api_v8.cc

cjihrig

LGTM with a question.

src/js_native_api_v8.cc

doc/api/n-api.md

src/js_native_api_v8.cc

gabrielschulhof

I believe the error handling is inconsistent with the rest of N-API.

src/js_native_api_v8.cc

gabrielschulhof · 2019-10-01T23:04:39Z

It looks like several inconsistencies have crept into how we handle errors in N-API 😕 Although it's too late to say this now, I don't believe N-API should itself throw on error. We should leave that option up to the caller.

I believe it is sufficient to return napi_invalid_arg in this case.

src/js_native_api_v8.cc

gabrielschulhof · 2019-10-01T23:25:31Z

In this PR we also have the option of adding one or more new napi_status value(s) to return upon failure. Such a change would not be semver-major.

doc/api/n-api.md

gabrielschulhof · 2019-10-01T23:30:14Z

Furthermore, if we do decide to throw, IMO we should return napi_pending_exception.

gabrielschulhof · 2019-10-01T23:44:41Z

Can we please call the function napi_detach_arraybuffer? We've mostly kept an imperative tone throughout the API so far:
napi_create_arraybuffer
napi_get_arraybuffer_info
napi_do_this
napi_do_that

legendecas · 2019-10-02T13:48:21Z

@gabrielschulhof Is there any suggestion on distinguishing napi_invalid_args in the case with different causes?

I noticed that napi_extended_error_info has a field error_message but we did not utilize it to supply additional information about the error but a description of the napi_status.

mhdawson · 2019-10-03T21:28:01Z

I'd agree with @gabrielschulhof that we should avoid throwing

legendecas · 2019-10-04T17:36:51Z

Just updated the patch to add two new napi_status and prevent from throwing:

napi_arraybuffer_expected, and
napi_detachable_arraybuffer_expected

Also since if an internal storage arraybuffer is detachable was not defined in ECMA spec, I've added a note in the document that requiring an external arraybuffer to be detached is an engine-specific behavior.

/cc @gabrielschulhof @mhdawson @cjihrig

legendecas · 2020-04-22T15:51:21Z

@mafintosh it seems very fortunate that v10 maintenance start date has been delayed to 2020-05-19. I'm willing to seek to land this patch on v10.

mafintosh · 2020-04-22T17:28:52Z

@legendecas ah massive thanks! really appreciate it. If we can help somehow let me know.

As ArrayBuffer#detach is an ecma spec operation ([Section 24.1.1.3](https://tc39.es/ecma262/#sec-detacharraybuffer)), it might be good to have it in N-API. Fixes nodejs#29674 PR-URL: nodejs#29768 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

As ArrayBuffer#detach is an ecma spec operation ([Section 24.1.1.3](https://tc39.es/ecma262/#sec-detacharraybuffer)), it might be good to have it in N-API. Fixes: #29674 PR-URL: #29768 Backport-PR-URL: #33061 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

Notable changes: - deps: - upgrade openssl sources to 1.1.1g (Hassaan Pasha) [#32982](#32982) - n-api: * add `napi_detach_arraybuffer` (legendecas) [#29768](#29768)

Notable changes: - deps: - upgrade openssl sources to 1.1.1g (Hassaan Pasha) #32982 - n-api: * add `napi_detach_arraybuffer` (legendecas) #29768

Notable changes: - deps: - upgrade openssl sources to 1.1.1g (Hassaan Pasha) #32982 - n-api: - add `napi_detach_arraybuffer` (legendecas) #29768 PR-URL: #34170

Notable changes: - deps: - upgrade npm to 6.14.6 (claudiahdz) #34246 - upgrade openssl sources to 1.1.1g (Hassaan Pasha) #32982 - n-api: - add `napi_detach_arraybuffer` (legendecas) #29768 PR-URL: #34170

Refs: nodejs/node#29768 Refs: nodejs/node#30613

Refs: nodejs/node#29768 Refs: nodejs/node#30613 PR-URL: #659 Refs: nodejs/node#29768 Refs: nodejs/node#30613 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

Refs: nodejs/node#29768 Refs: nodejs/node#30613 PR-URL: nodejs#659 Refs: nodejs/node#29768 Refs: nodejs/node#30613 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

Refs: nodejs/node#29768 Refs: nodejs/node#30613 PR-URL: nodejs/node-addon-api#659 Refs: nodejs/node#29768 Refs: nodejs/node#30613 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 30, 2019

addaleax approved these changes Sep 30, 2019

View reviewed changes

doc/api/n-api.md Outdated Show resolved Hide resolved

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

devnexen approved these changes Sep 30, 2019

View reviewed changes

cjihrig approved these changes Sep 30, 2019

View reviewed changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

jasnell reviewed Sep 30, 2019

View reviewed changes

doc/api/n-api.md Outdated Show resolved Hide resolved

jasnell approved these changes Sep 30, 2019

View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 30, 2019

vsemozhetbyt reviewed Sep 30, 2019

View reviewed changes

doc/api/n-api.md Outdated Show resolved Hide resolved

mhdawson reviewed Sep 30, 2019

View reviewed changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

legendecas mentioned this pull request Oct 1, 2019

doc: reuse link indexes for n-api.md #29787

Closed

3 tasks

gabrielschulhof requested changes Oct 1, 2019

View reviewed changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

gabrielschulhof reviewed Oct 1, 2019

View reviewed changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

gabrielschulhof reviewed Oct 1, 2019

View reviewed changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

devsnek reviewed Oct 1, 2019

View reviewed changes

doc/api/n-api.md Outdated Show resolved Hide resolved

legendecas force-pushed the issue-29674 branch 2 times, most recently from 1000a3e to 96dd798 Compare October 2, 2019 13:43

legendecas changed the title ~~n-api: add napi_arraybuffer_detach~~ n-api: add napi_detach_arraybuffer Oct 2, 2019

legendecas force-pushed the issue-29674 branch from 96dd798 to 415c8c7 Compare October 2, 2019 13:52

legendecas force-pushed the issue-29674 branch from 415c8c7 to 51742d4 Compare October 4, 2019 17:32

legendecas mentioned this pull request Oct 4, 2019

src,n-api: throwing error with napi_pending_exception status #29847

Closed

4 tasks

legendecas added the lts-watch-v10.x label Apr 22, 2020

legendecas added the backport-requested-v10.x label Apr 23, 2020

legendecas mentioned this pull request Apr 25, 2020

[v10.x] n-api: backport ArrayBuffer detaching APIs #33061

Closed

legendecas added the backport-open-v10.x label Apr 26, 2020

richardlau mentioned this pull request Jul 2, 2020

v10.22.0 proposal #34170

Merged

4 tasks

richardlau added a commit that referenced this pull request Jul 2, 2020

2020-07-21, Version 10.22.0 'Dubnium' (LTS)

5cf3b31

Notable changes: - deps: - upgrade openssl sources to 1.1.1g (Hassaan Pasha) [#32982](#32982) - n-api: * add `napi_detach_arraybuffer` (legendecas) [#29768](#29768)

richardlau added a commit that referenced this pull request Jul 6, 2020

2020-07-21, Version 10.22.0 'Dubnium' (LTS)

6a26d1e

Notable changes: - deps: - upgrade openssl sources to 1.1.1g (Hassaan Pasha) #32982 - n-api: * add `napi_detach_arraybuffer` (legendecas) #29768

richardlau added a commit that referenced this pull request Jul 7, 2020

2020-07-21, Version 10.22.0 'Dubnium' (LTS)

b4ac3b1

Notable changes: - deps: - upgrade openssl sources to 1.1.1g (Hassaan Pasha) #32982 - n-api: - add `napi_detach_arraybuffer` (legendecas) #29768 PR-URL: #34170

richardlau added backported-to-v10.x and removed lts-watch-v10.x labels Oct 7, 2020

tniessen added a commit to tniessen/node-addon-api that referenced this pull request Nov 5, 2020

Add ArrayBuffer::Detach() and ::IsDetached()

ad9f5b0

Refs: nodejs/node#29768 Refs: nodejs/node#30613

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: add `napi_detach_arraybuffer` #29768

n-api: add `napi_detach_arraybuffer` #29768

legendecas commented Sep 30, 2019 •

edited

cjihrig left a comment

gabrielschulhof left a comment

gabrielschulhof commented Oct 1, 2019

gabrielschulhof commented Oct 1, 2019 •

edited

gabrielschulhof commented Oct 1, 2019

gabrielschulhof commented Oct 1, 2019

legendecas commented Oct 2, 2019

mhdawson commented Oct 3, 2019

legendecas commented Oct 4, 2019

legendecas commented Apr 22, 2020

mafintosh commented Apr 22, 2020

n-api: add napi_detach_arraybuffer #29768

n-api: add napi_detach_arraybuffer #29768

Conversation

legendecas commented Sep 30, 2019 • edited

Checklist

cjihrig left a comment

Choose a reason for hiding this comment

gabrielschulhof left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Oct 1, 2019

gabrielschulhof commented Oct 1, 2019 • edited

gabrielschulhof commented Oct 1, 2019

gabrielschulhof commented Oct 1, 2019

legendecas commented Oct 2, 2019

mhdawson commented Oct 3, 2019

legendecas commented Oct 4, 2019

legendecas commented Apr 22, 2020

mafintosh commented Apr 22, 2020

n-api: add `napi_detach_arraybuffer` #29768

n-api: add `napi_detach_arraybuffer` #29768

legendecas commented Sep 30, 2019 •

edited

gabrielschulhof commented Oct 1, 2019 •

edited