Add napi_finalize override to ObjectWrap

[mikepricedev]

By implementing the release of the native instance, ObjectWrap takes an opinionated approach to the finalize_cb in N-API napi_wrap. This is arguably the correct default as the central theme of the Object Wrap methods in N-API is encapsulation. Specifically tying the properties and lifetimes of a native instance to a JS object.

However, because this default is not optional/overridable, it excludes access to more complex finalization scenarios allowed by N-API. Specifically when access to the napi_env is required. Because node-addon-api is meant to be a thin convenience wrapper, it follows that some method of access to the underlying N-API should exist where defaults are not merely a difference in syntax and style between C++ and C.

I've added ObjectWrap::OverrideFinalizeCallback to accomplish this in regards to the default napi_finalize in ObjectWrap. I prefixed with "Override" as opposed to "Set" in order to make it clear that there is a default behavior that is being overridden. I also stated in the documentation that the responsibility of freeing the native instance is passed to the user defined napi_finalize callback when ObjectWrap::OverrideFinalizeCallback is used.

[mhdawson]

@raptor9g I'm wondering if simply making FinalizeCallback overridable versus private would be simpler and achieve the same goal?

[gabrielschulhof]

We could also provide a protected instance method that gets called before the implementation does delete instance:

diff --git a/napi-inl.h b/napi-inl.h
index 0db3c98..6223f41 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -3009,6 +3009,11 @@ inline Function ObjectWrap<T>::DefineClass(
            data);
 }
 
+template <typename T>
+inline void Finalize(napi_env env) {
+  (void) env;
+}
+
 template <typename T>
 inline ClassPropertyDescriptor<T> ObjectWrap<T>::StaticMethod(
     const char* utf8name,
@@ -3400,8 +3405,9 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(
 }
 
 template <typename T>
-inline void ObjectWrap<T>::FinalizeCallback(napi_env /*env*/, void* data, void* /*hint*/) {
+inline void ObjectWrap<T>::FinalizeCallback(napi_env env, void* data, void* /*hint*/) {
   T* instance = reinterpret_cast<T*>(data);
+  instance->Finalize(env);
   delete instance;
 }
 
diff --git a/napi.h b/napi.h
index c194641..72c3590 100644
--- a/napi.h
+++ b/napi.h
@@ -1658,6 +1658,9 @@ namespace Napi {
                                             Napi::Value value,
                                             napi_property_attributes attributes = napi_default);
 
+  protected:
+    virtual void Finalize(napi_env env);
+
   private:
     static napi_value ConstructorCallbackWrapper(napi_env env, napi_callback_info info);
     static napi_value StaticVoidMethodCallbackWrapper(napi_env env, napi_callback_info info);

[mikepricedev]

@raptor9g I'm wondering if simply making FinalizeCallback overridable versus private would be simpler and achieve the same goal?

@mhdawson Yeah that would work great! Since ObjectWrap::FinalizeCallback is static, the only change would be to look it up in ObjectWrap ctor as &T::FinalizeCallback as apposed to FinalizeCallback, then some documentation letting users know they can add an optional static FinalizeCallback to their classes deriving from ObjectWrap?

diff --git a/napi-inl.h b/napi-inl.h
index 0db3c98..5fd9125 100644
--- a/napi-inl.h
+++ b/napi-inl.h
@@ -2879,7 +2879,7 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
   napi_status status;
   napi_ref ref;
   T* instance = static_cast<T*>(this);
-  status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
+  status = napi_wrap(env, wrapper, instance, &T::FinalizeCallback, nullptr, &ref);
   NAPI_THROW_IF_FAILED_VOID(env, status);
 
   Reference<Object>* instanceRef = instance;

Here's an implementation.

[mikepricedev]

@gabrielschulhof That would work for my specific instance. Given that the override is providing deeper access to the underlying N-API finalizer implementation, at some point should it give full access to the implementation?

[gabrielschulhof]

@raptor9g node-addon-api and N-API are fully mixable. Users wishing to fully control the connection between a native instance and the corresponding JS object can use napi_wrap() directly:

Napi::Value SomeBinding(const Napi::CallbackInfo& info) {
  Napi::Object obj = Napi::Object::New(info.Env());
  NativeStuff* stuff = new NativeStuff();
  NAPI_THROW_IF_FAILED(env,
                       napi_wrap(info.Env(),
                                 obj,
                                 stuff,
                                 DeleteNativeStuff,
                                 NULL,
                                 NULL),
                       Napi::Value());
  return obj;
}

IMO the value of this PR is that users need not give up on the ObjectWrap abstraction if they need the napi_env env or, equivalently, the Napi::Env env in their destructor, because this PR opens up the env-sensitive portion of the teardown to users.

One kind of deeper access that we might want to provide in the future is to the allocator responsible for creating the native instance. Currently we hard-code that to be new/delete. We could expose this pair if needed.

IIUC with your implementation as given in this PR as well as the implementation you point to in the link there is no opportunity to chain up to the parent class to actually perform the delete instance, and so the responsibility for performing the deletion falls to the user. This is undesirable, because it closes the life cycle of the native object outside the implementation whereas the native object commenced its life cycle inside the implementation. In contrast, the instance method approach, although having a greater diff footprint than the implementation you point to in the link, leaves the deletion in the implementation while still providing the user the opportunity to perform env-sensitive cleanup immediately prior to deletion.

[mikepricedev]

IIUC with your implementation as given in this PR as well as the implementation you point to in the link there is no opportunity to chain up to the parent class to actually perform the delete instance, and so the responsibility for performing the deletion falls to the user.

@gabrielschulhof This is a really good point. And I like your approach to adding a virtual instance method bc it tells you, "hey there something here you can implement." And because it's not a pure virtual, it also indicates that it is optional.

As to the deletion, I noticed while I was reading the documentation in N-API for napi_wrap that the finalizer is optional. It seems that while the beginning of the native and JS instance's lifecycles are tied together, napi_wrap opens the door for the end of the JS and native instance's lifecycles to diverge. Should this be allowed by some mechanism in ObjectWrap? My approach allows for the ending lifecycles to diverge, but, as you pointed out, forces you to deal with the deletion even if all you want is access to the napi_env. It's all or nothing.

[mhdawson]

@raptor9g

My approach allows for the ending lifecycles to diverge

Do you have some use cases in mind where this is useful? Other than that I like @gabrielschulhof 's rationale of better managing the lifecycle while allowing "additional" work if the subclass need to do it.

[gabrielschulhof]

@raptor9g @mhdawson I was thinking that maybe wrapping a static native instance while retaining the ObjectWrap abstraction might be interesting, however, IMO for the scope of this PR we need not look into that.

[mikepricedev]

@mhdawson I do not have a specific use case for extending the native instance lifespan outside of a trivial "save native state/do work later." My specific use case only requires access to the napi_env. I need to call some N-API methods when/if the JS-Object is gc'd.

[gabrielschulhof]

@raptor9g would you then mind modifying the PR to add the Finalize() virtual method?

[mikepricedev]

@raptor9g would you then mind modifying the PR to add the Finalize() virtual method?

@gabrielschulhof Absolutely, just pushed it. I also cited you in the commit, as I basically just typed in what you proposed. I want to make sure I did that correctly.

[gabrielschulhof]

Could you please add a test which performs some cleanup that requires N-API? For example, it could call a callback that is provided at construction time, and to which a reference is retained throughout the life cycle of the wrapped object.

[mikepricedev]

Could you please add a test which performs some cleanup that requires N-API? For example, it could call a callback that is provided at construction time, and to which a reference is retained throughout the life cycle of the wrapped object.

@gabrielschulhof Pushed a commit with a test. Let me know if that works?

[gabrielschulhof]

LGTM % nits.

[mhdawson]

LGTM

[gabrielschulhof]

LGTM

[gabrielschulhof]

CI:

version	job	result
13	https://ci.nodejs.org/job/node-test-node-addon-api-new/306/	✗
12	https://ci.nodejs.org/job/node-test-node-addon-api-new/307/	✗
10	https://ci.nodejs.org/job/node-test-node-addon-api-new/308/	✗
8	https://ci.nodejs.org/job/node-test-node-addon-api-new/309/	✗

[mhdawson]

@gabrielschulhof based on the failures

inline void ObjectWrap<T>::Finalize(Napi::Env env) {}

probably needs to be

inline void ObjectWrap<T>::Finalize(Napi::Env /*env*/) {}

[gabrielschulhof]

@mhdawson ... and there also needs to be a destructor for ObjectWrap declared virtual in napi.h but implemented as empty in napi-inl.h.

[gabrielschulhof]

@raptor9g could you please add these?

[mikepricedev]

@raptor9g could you please add these?

Yes sir. They have been pushed.

[gabrielschulhof]

Thank you kindly, good sir!

[gabrielschulhof]

CI:

Version	Job	Status
13	https://ci.nodejs.org/job/node-test-node-addon-api-new/310/	✓
12	https://ci.nodejs.org/job/node-test-node-addon-api-new/311/	✓
10	https://ci.nodejs.org/job/node-test-node-addon-api-new/312/	✓
8	https://ci.nodejs.org/job/node-test-node-addon-api-new/313/	✓

[gabrielschulhof]

Landed in c3c8814.