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

n-api: add napi_detach_arraybuffer #29768

Closed
wants to merge 1 commit into from
Closed

n-api: add napi_detach_arraybuffer #29768

wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Sep 30, 2019 •
edited
Loading

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
addaleax
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
cjihrig
cjihrig approved these changes Sep 30, 2019
View reviewed changes
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
jasnell
jasnell reviewed Sep 30, 2019
View reviewed changes
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
vsemozhetbyt
vsemozhetbyt reviewed Sep 30, 2019
View reviewed changes
doc/api/n-api.md Outdated Show resolved Hide resolved
mhdawson
mhdawson reviewed Sep 30, 2019
View reviewed changes
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
@legendecas legendecas mentioned this pull request Oct 1, 2019
Closed
3 tasks
gabrielschulhof
gabrielschulhof requested changes Oct 1, 2019
View reviewed changes
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 •
edited
Loading

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.

devsnek
devsnek reviewed Oct 1, 2019
View reviewed changes
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

@legendecas legendecas mentioned this pull request Oct 4, 2019
Closed
4 tasks
@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 legendecas mentioned this pull request Apr 25, 2020
Closed
legendecas added a commit to legendecas/node that referenced this pull request Jul 1, 2020
@legendecas
n-api: add napi_detach_arraybuffer
2243556 
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
@legendecas @richardlau
n-api: add napi_detach_arraybuffer
961598b 
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
Merged
4 tasks
richardlau added a commit that referenced this pull request Jul 2, 2020
@richardlau
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
@richardlau
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
@richardlau
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 a commit that referenced this pull request Jul 13, 2020
@richardlau
2020-07-21, Version 10.22.0 'Dubnium' (LTS)
eaa9e4d 
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
@richardlau
2020-07-21, Version 10.22.0 'Dubnium' (LTS)
c5215d0 
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
@richardlau @BethGriggs
2020-07-21, Version 10.22.0 'Dubnium' (LTS)
95770df 
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
@richardlau @cjihrig
2020-07-21, Version 10.22.0 'Dubnium' (LTS)
8ad4920 
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
@tniessen
Add ArrayBuffer::Detach() and ::IsDetached()
ad9f5b0 
Refs: nodejs/node#29768
Refs: nodejs/node#30613
mhdawson pushed a commit to nodejs/node-addon-api that referenced this pull request Nov 5, 2020
@tniessen @mhdawson
src: add ArrayBuffer::Detach() and ::IsDetached()
c9563ca 
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
@tniessen @Superlokkus
src: add ArrayBuffer::Detach() and ::IsDetached()
586e584 
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
@kevindavies8
src: add ArrayBuffer::Detach() and ::IsDetached()
ae16785 
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
@Marlyfleitas
src: add ArrayBuffer::Detach() and ::IsDetached()
e538d3c 
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
@wroy7860
src: add ArrayBuffer::Detach() and ::IsDetached()
f0588bc 
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
src: add ArrayBuffer::Detach() and ::IsDetached()
39f355a 
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
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@mhdawson mhdawson mhdawson left review comments

@devsnek devsnek devsnek left review comments

@addaleax addaleax addaleax approved these changes

@devnexen devnexen devnexen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add napi_buffer_detach()