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

n-api: add napi_detach_arraybuffer #29768

Closed
wants to merge 1 commit into from
Closed

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Sep 30, 2019

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

@nodejs-github-bot nodejs-github-bot added the c++ label Sep 30, 2019
doc/api/n-api.md Outdated Show resolved Hide resolved
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@cjihrig cjihrig left a comment

LGTM with a question.

src/js_native_api_v8.cc Outdated Show resolved Hide resolved
doc/api/n-api.md Outdated Show resolved Hide resolved
@jasnell jasnell added the semver-minor label Sep 30, 2019
doc/api/n-api.md Outdated Show resolved Hide resolved
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

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

src/js_native_api_v8.cc Outdated Show resolved Hide resolved
@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Oct 1, 2019

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 Outdated Show resolved Hide resolved
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Oct 1, 2019

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 Outdated Show resolved Hide resolved
@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Oct 1, 2019

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

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Oct 1, 2019

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 legendecas force-pushed the issue-29674 branch 2 times, most recently from 1000a3e to 96dd798 Compare Oct 2, 2019
@legendecas legendecas changed the title n-api: add napi_arraybuffer_detach n-api: add napi_detach_arraybuffer Oct 2, 2019
@legendecas
Copy link
Member Author

@legendecas legendecas commented Oct 2, 2019

@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
Copy link
Member

@mhdawson mhdawson commented Oct 3, 2019

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

@legendecas
Copy link
Member Author

@legendecas legendecas commented Oct 4, 2019

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

tniessen added a commit to tniessen/node-addon-api that referenced this issue Jan 26, 2020
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
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
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>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
@mafintosh
Copy link
Member

@mafintosh mafintosh commented Apr 22, 2020

Anyway to get this backported to 10? We need this is our libsodium bindings, to deallocate secure memory safely.

@legendecas
Copy link
Member Author

@legendecas legendecas commented Apr 22, 2020

@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
Copy link
Member

@mafintosh mafintosh commented Apr 22, 2020

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

legendecas added a commit to legendecas/node that referenced this issue Jul 1, 2020
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>
richardlau pushed a commit that referenced this issue Jul 1, 2020
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>
@richardlau richardlau mentioned this pull request Jul 2, 2020
4 tasks
richardlau added a commit that referenced this issue Jul 2, 2020
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 issue Jul 6, 2020
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 issue Jul 7, 2020
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 a commit that referenced this issue Jul 13, 2020
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
richardlau added a commit that referenced this issue Jul 15, 2020
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
BethGriggs pushed a commit that referenced this issue Jul 21, 2020
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
cjihrig pushed a commit that referenced this issue Jul 23, 2020
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
tniessen added a commit to tniessen/node-addon-api that referenced this issue Nov 5, 2020
mhdawson pushed a commit to nodejs/node-addon-api that referenced this issue Nov 5, 2020
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>
Superlokkus pushed a commit to Superlokkus/node-addon-api that referenced this issue Nov 20, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready c++ node-api semver-minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.