n-api: add ability to remove a wrapping

[gabrielschulhof]

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]

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

[gabrielschulhof]

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

[jasongin]

Why shouldn't it?

Either way, the behavior needs to be documented.

[gabrielschulhof]

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]

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.)

[gabrielschulhof]

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().

[jasongin]

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?

[gabrielschulhof]

@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]

Is a try-catch needed here?

[gabrielschulhof]

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.

[jasongin]

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

[gabrielschulhof]

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

[addaleax]

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.

[gabrielschulhof]

@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.

[gabrielschulhof]

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

[jasongin]

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

[addaleax]

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

[jasongin]

Oh. Good to know. :)

[gabrielschulhof]

@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]

@addaleax

[gabrielschulhof]

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]

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

[gabrielschulhof]

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

[mhdawson]

AIX issue in CI was know unrelated issue.

[mhdawson]

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]

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]

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

[gabrielschulhof]

Landed in 70664bf.