n-api: add ability to remove a wrapping #14658

gabrielschulhof · 2017-08-07T06:15:04Z

Calling napi_wrap() twice on the same object has the result of returning
napi_invalid_arg on the second call. However, sometimes it is necessary
to replace the native pointer associated with a given object. This new
API allows one to remove an existing pointer, returning the object to
its pristine, non-wrapped state.

Fixes: nodejs/abi-stable-node#266

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

Affected core subsystem(s)

n-api

jasongin · 2017-08-07T18:45:22Z

doc/api/n-api.md

+
+Retrieves a native instance that was previously wrapped in the JavaScript
+object `js_object` using `napi_wrap()` and removes the wrapping, thereby
+restoring the JavaScript object's prototype chain.


Does this invoke the finalizer callback of the previous wrapping? Should it?

My intention was that it not call the finalizer callback. I need to test this.

Why shouldn't it?

Either way, the behavior needs to be documented.

If it calls the finalizer callback it's kinda useless to return the pointer, since it's already freed. I guess my concept was that by removing the wrapping, you resume responsibility for freeing the pointer, having previously assigned such responsibility to the VM.

... and I need to update the documentation ...

jasongin · 2017-08-07T18:47:03Z

doc/api/n-api.md

+
+ - `[in] env`: The environment that the API is invoked under.
+ - `[in] js_object`: The object associated with the native instance.
+ - `[out] result`: Pointer to the wrapped native instance.


Why also return the pointer here? That is already available via napi_wrap(). Or, maybe these two APIs could be combined, with a boolean parameter that indicates whether to remove it at the same time. (I'm not sure I actually prefer that though.)

I guess it's to avoid having to make two API calls. In nodejs/abi-stable-node#266 @sampsongao is trying to do a napi_unwrap() followed unconditionally by a napi_wrap(). Knowing that an unconditional napi_wrap() will follow would allow him to drop in napi_remove_wrap() instead of napi_unwrap().

If you're concerned about performance, why not make it work in one napi_wrap() call? Maybe a boolean to allow overwriting a previous wrap? Or return a previous wrap value?

@mhdawson wanted an explicit napi_remove_wrap(), but, I guess an explicit true/false parameter is also, well, explicit.

If we do this within napi_unwrap(), though, we're treading into semver-major territory - which I OK while in experimental, but still, to be discussed, right?

jasongin · 2017-08-07T18:48:14Z

src/node_api.cc


+napi_status napi_remove_wrap(napi_env env, napi_value obj, void** result) {
+  NAPI_PREAMBLE(env);


Is a try-catch needed here?

I took it as a hint from V8 that an exception may be thrown in ->SetPrototype() because it returns a v8::Maybe<bool> - much like .

In fact, we should re-examine how we deal with v8::Maybe(Local)? return values as we concurrently deal with exceptions. I get the impression that an empty/nothing return value happens iff there was an exception. If that's the case then we should perhaps not return napi_generic_failure if the v8::Maybe(Local)? is empty, but we should rather check for an exception first and return napi_exception_pending if we find one, and only return napi_generic_failure if we don't.

I don't think a v8::Maybe return type always means there is a possibility of an exception.

OK, then we need to check whether there is such a possibility.

Most of the Maybe errors can be triggered through Proxies; for example, SetPrototype() fails when a setPrototype() trap throws. It can also throw e.g. when you try to set up a circular prototype chain or something like that, but I don’t think that would happen here.

@addaleax out of curiosity and AFAUK, does V8 have any APIs which return v8::Maybes or v8::MaybeLocals even though there is no chance that an exception will be thrown in their execution?

The reason I ask is that the only ones I've seen are property get/set/has/delete and SetPrototype, all of which can throw exceptions.

Wait, NM. v8::String::NewFromUtf8.

Yes. I was also thinking of the number APIs like v8::Value::Int32Value(context), that returns a maybe but doesn't throw.

@jasongin But that returns a Maybe because it can throw, Symbol.toPrimitive says hi. ;)

Oh. Good to know. :)

gabrielschulhof · 2017-08-08T22:11:35Z

@jasongin @nodejs/addon-api
Here's what I have so far:

napi_remove_wrap() is a separate function that does the same thing as napi_unwrap(), except it also restores the JavaScript object to its pristine state so it can be wrapped again.
After calling napi_remove_wrap() the user becomes responsible for freeing the pointer that was being handled by a finalize callback.
We need a NAPI_PREAMBLE(env) for napi_remove_wrap() because setting the prototype may throw. In fact, we already have NAPI_PREAMBLE(env) for napi_wrap(), which also manipulates the prototype chain, so I guess the need for NAPI_PREAMBLE(env) in napi_remove_wrap() is for the same reason.

Question:

Do we want to render the functionality provided by napi_remove_wrap() as a new boolean parameter to napi_unwrap() such that true indicates that you wish to retrieve the pointer and detach it from the JavaScript object, and false indicates that you merely wish to retrieve the pointer? This would entail an additional ABI break - which is theoretically OK(ish) in experimental.

gabrielschulhof · 2017-08-08T22:12:12Z

@addaleax

gabrielschulhof · 2017-08-10T17:50:32Z

During tonight's N-API meeting we decided to avoid the ABI break. Let's keep napi_remove_wrap() as a separate function.

mhdawson

LGTM

gabrielschulhof · 2017-08-17T07:39:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9702/

Calling napi_wrap() twice on the same object has the result of returning napi_invalid_arg on the second call. However, sometimes it is necessary to replace the native pointer associated with a given object. This new API allows one to remove an existing pointer, returning the object to its pristine, non-wrapped state. Fixes: nodejs/abi-stable-node#266

Mention that pointer ownership returns to the application after napi_remove_wrap() and add test making sure the finalize callback gets called when it needs to, and that it does not get called when it shouldn't.

gabrielschulhof · 2017-08-17T12:39:45Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/9711/

mhdawson · 2017-08-18T14:02:18Z

AIX issue in CI was know unrelated issue.

mhdawson · 2017-08-18T14:05:44Z

Arm failure was a build failure so likely unrelated, but due to failures on other platforms lets try the CI again: https://ci.nodejs.org/job/node-test-commit/11887/

mhdawson · 2017-08-18T16:24:41Z

OSX and fips failures in latest run were infra related and they had run/passed in earlier CI runs to validate this PR so I think we are good to land.

mhdawson · 2017-08-18T16:26:11Z

@addaleax @jasongin I think your comments/concerns have been addressed so I'll plan to land on Monday unless I hear otherwise from you.

Calling napi_wrap() twice on the same object has the result of returning napi_invalid_arg on the second call. However, sometimes it is necessary to replace the native pointer associated with a given object. This new API allows one to remove an existing pointer, returning the object to its pristine, non-wrapped state. PR-URL: #14658 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Fixes: nodejs/abi-stable-node#266

gabrielschulhof · 2017-08-22T09:34:57Z

Landed in 70664bf.

Calling napi_wrap() twice on the same object has the result of returning napi_invalid_arg on the second call. However, sometimes it is necessary to replace the native pointer associated with a given object. This new API allows one to remove an existing pointer, returning the object to its pristine, non-wrapped state. PR-URL: nodejs/node#14658 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Fixes: nodejs/abi-stable-node#266

Calling napi_wrap() twice on the same object has the result of returning napi_invalid_arg on the second call. However, sometimes it is necessary to replace the native pointer associated with a given object. This new API allows one to remove an existing pointer, returning the object to its pristine, non-wrapped state. PR-URL: #14658 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Fixes: nodejs/abi-stable-node#266

Calling napi_wrap() twice on the same object has the result of returning napi_invalid_arg on the second call. However, sometimes it is necessary to replace the native pointer associated with a given object. This new API allows one to remove an existing pointer, returning the object to its pristine, non-wrapped state. PR-URL: nodejs#14658 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Fixes: nodejs/abi-stable-node#266

Calling napi_wrap() twice on the same object has the result of returning napi_invalid_arg on the second call. However, sometimes it is necessary to replace the native pointer associated with a given object. This new API allows one to remove an existing pointer, returning the object to its pristine, non-wrapped state. Backport-PR-URL: #19447 PR-URL: #14658 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Fixes: nodejs/abi-stable-node#266

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Aug 7, 2017

jasongin reviewed Aug 7, 2017

View reviewed changes

gabrielschulhof force-pushed the napi-remove-wrap branch from 096a29b to a164e89 Compare August 8, 2017 21:59

gabrielschulhof added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 9, 2017

mhdawson approved these changes Aug 14, 2017

View reviewed changes

Gabriel Schulhof added 3 commits August 17, 2017 14:34

squash: update documentation and improve test

0f07c2f

Mention that pointer ownership returns to the application after napi_remove_wrap() and add test making sure the finalize callback gets called when it needs to, and that it does not get called when it shouldn't.

squash: fix linter issues

24126f4

gabrielschulhof force-pushed the napi-remove-wrap branch from a164e89 to 24126f4 Compare August 17, 2017 12:37

gabrielschulhof closed this Aug 22, 2017

gabrielschulhof deleted the napi-remove-wrap branch August 22, 2017 09:36

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Jan 11, 2018

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: add ability to remove a wrapping #14658

n-api: add ability to remove a wrapping #14658

gabrielschulhof commented Aug 7, 2017

jasongin Aug 7, 2017

gabrielschulhof Aug 7, 2017

jasongin Aug 7, 2017

gabrielschulhof Aug 7, 2017

jasongin Aug 7, 2017

gabrielschulhof Aug 7, 2017

jasongin Aug 7, 2017 •

edited

gabrielschulhof Aug 7, 2017

jasongin Aug 7, 2017

gabrielschulhof Aug 7, 2017

jasongin Aug 7, 2017 •

edited

gabrielschulhof Aug 7, 2017

addaleax Aug 7, 2017

gabrielschulhof Aug 7, 2017

gabrielschulhof Aug 7, 2017

jasongin Aug 7, 2017

addaleax Aug 7, 2017 •

edited

jasongin Aug 7, 2017

gabrielschulhof commented Aug 8, 2017

gabrielschulhof commented Aug 8, 2017

gabrielschulhof commented Aug 10, 2017

mhdawson left a comment

gabrielschulhof commented Aug 17, 2017

gabrielschulhof commented Aug 17, 2017

mhdawson commented Aug 18, 2017

mhdawson commented Aug 18, 2017 •

edited

mhdawson commented Aug 18, 2017

mhdawson commented Aug 18, 2017

gabrielschulhof commented Aug 22, 2017


		napi_status napi_remove_wrap(napi_env env, napi_value obj, void** result) {
		NAPI_PREAMBLE(env);

n-api: add ability to remove a wrapping #14658

n-api: add ability to remove a wrapping #14658

Conversation

gabrielschulhof commented Aug 7, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasongin Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasongin Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Aug 8, 2017

gabrielschulhof commented Aug 8, 2017

gabrielschulhof commented Aug 10, 2017

mhdawson left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Aug 17, 2017

gabrielschulhof commented Aug 17, 2017

mhdawson commented Aug 18, 2017

mhdawson commented Aug 18, 2017 • edited

mhdawson commented Aug 18, 2017

mhdawson commented Aug 18, 2017

gabrielschulhof commented Aug 22, 2017

jasongin Aug 7, 2017 •

edited

jasongin Aug 7, 2017 •

edited

addaleax Aug 7, 2017 •

edited

mhdawson commented Aug 18, 2017 •

edited