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

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++ Issues and PRs that require attention from people who are familiar with 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

Choose a reason for hiding this comment

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

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 PRs that contain new features and should be released in the next minor version. 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

Choose a reason for hiding this comment

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

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

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.

@gabrielschulhof
Copy link
Contributor

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

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

@gabrielschulhof
Copy link
Contributor

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 October 2, 2019 13:43
@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

@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 commented Oct 3, 2019

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

@legendecas
Copy link
Member Author

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

@mafintosh
Copy link
Member

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

@legendecas
Copy link
Member Author

@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

@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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request Nov 5, 2020
mhdawson pushed a commit to nodejs/node-addon-api that referenced this pull request 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 pull request 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>
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
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>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
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>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
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>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

Add napi_buffer_detach()