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

Napi::Reference not copyable, but semantically could be copyable #301

Closed
DaAitch opened this issue Jul 15, 2018 · 9 comments
Closed

Napi::Reference not copyable, but semantically could be copyable #301

DaAitch opened this issue Jul 15, 2018 · 9 comments

Comments

@DaAitch
Copy link
Contributor

DaAitch commented Jul 15, 2018

Hi,

I don't understand why Napi::Reference can't be copied, although for Napi::Error it is allowed, implemented as protected. From technical point of view, the C-API allows create multiple reference holders of one value. When using lambdas captures we need copyable types.

Look at the following example. A 3rd party lib allocates some memory we want to store in an Napi::External with a finalizer. We need to reference handle to overlive lambda where it is freed and after handle is unreferenced.

// variable
Napi::Reference<Napi::External<H> > handle_; // H is handle type

Napi::Value MyClass::Alloc(const Napi::CallbackInfo& info) {
  Napi::EscapableHandleScope scope(info.Env());
  
  unsigned int size = info[0].ToNumber().Uint32Value();
  unsigned char* mem = lib_alloc(handle_.Value().Data(), size); // 3rd party alloc

  // lambda not copyable, because capture are not copyable

  // Either A)
  // create new Reference
  return scope.Escape(DMA<unsigned char>::constructor.New({
    // Napi::Persistent is noncopyable 
    Napi::External<unsigned char>::New(info.Env(), mem, [size, handle = Napi::Persistent(handle_.Value())](Napi::Env env, unsigned char* data) {
      lib_free(handle.Value().Data(), data, size); // 3rd party free
    })
  }));

  // Or B)
  // copy Reference
  return scope.Escape(DMA<unsigned char>::constructor.New({
    // Napi::Persistent is noncopyable 
    Napi::External<unsigned char>::New(info.Env(), mem, [size, handle = handle_](Napi::Env env, unsigned char* data) {
      lib_free(handle.Value().Data(), data, size); // 3rd party free
    })
  }));
}

I think we should make Napi::Reference copyable.
What do you think?

Best Regards
Philipp

@mhdawson
Copy link
Member

@nodejs/abi-stable-node please comment.

@jasongin
Copy link
Member

Napi::Reference is equivalent to v8::Persistent (which may be "weak" or not). By default v8::Persistent is not copyable, though that restriction may be changed using a traits parameter. See CopyablePersistentTraits in v8.

My interpretation of the reasoning for that default behavior is that if a reference is freely copyable then it's much easier to lose track and leak the referenced value. I intentionally started Napi::Reference with that more conservative restriction, with the thought that we can loosen the restriction later if necessary, either via similar traits or some other API.

DaAitch added a commit to wildoak/node-addon-api that referenced this issue Aug 22, 2018
- implement copyable `Holder` holding a `Reference<T>`
- implement `Hold` function to hold any `Value`
- test using `Async` to demonstrate need of copyable references

Refs: nodejs#301
@DaAitch
Copy link
Contributor Author

DaAitch commented Aug 22, 2018

@jasongin I agree that it would not be very clear what copying a Reference<T> actually means.

Because of this, I thought of the idea of a Holder impl provided here wildoak@90294e4 .

The idea is to take a Napi::Value to where you need it, e.g. as capture in lambdas.
What do you think about it?

@DaAitch
Copy link
Contributor Author

DaAitch commented Dec 26, 2018

@jasongin I think this is still an issue that should be considered. Everytime I use lambdas or however need to keep a reference for using it later, I jump back to C-NAPI using napi_ref.

If Napi::Reference should be what v8::Persistent is, would it be worth starting a PR with a different class having that features, being also copyable?

Seen lot's of code where they introduce a new object on heap (e.g. via shared_ptr), which holds the reference and then can be moved/copied, but as N-API supports that refcount features, I don't want to create objects on the heap if not necessary.
I'd say that a Reference is like a shared_ptr. They should all live on the stack. When they're destructed, ref count is decremented.

So they are/have:

  • construct (napi_ref_*)
  • destruct (napi_unref_*)
  • movable (swap napi_ref value, no napi_* calls)
  • copyable (napi_ref_*)

DaAitch added a commit to DaAitch/node-addon-api that referenced this issue Dec 27, 2018
@DaAitch
Copy link
Contributor Author

DaAitch commented Dec 27, 2018

Did another implementation on this as Napi::SharedValueReference<T>.

pro:

  • copyable, movable references
  • no dynamic memory allocation
  • no pending references or leaks possible

contra:

  • global std::mutex synchronizing napi_*_reference_* API calls

Now this is possible:

auto&& ref = Napi::MakeShared(myObj);
std::thread([ref = std::move(ref)]() {
  auto&& result = heavyProcessing();
  auto&& data = new ts_data{std::move(ref), result};

  // target threadsafe_function has to delete data and automatically unref reference
  // => wait for `node-addon-api` supporting `napi_*_threadsafe_*` API
  napi_call_threadsafe_function(ts_func, data, napi_tsfn_nonblocking);
});

@github-actions
Copy link
Contributor

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

@mhdawson
Copy link
Member

@legendecas will take a look and lead the discussion in a future meeting.

@mhdawson
Copy link
Member

mhdawson commented Feb 5, 2021

@legendecas still looking at this one.

@legendecas
Copy link
Member

legendecas commented Feb 26, 2021

While playing around the Napi::Reference with threading libraries, I'm still hesitant on making Napi::Reference copyable -- it is dangerous and not safe to copying Napi::Reference around not on the creating thread. Most APIs of node-api/node-addon-api are not threadsafe, except those ones of ThreadSafeFunction, like napi_call_threadsafe_function. With C++ object ownership and lifetime management semantics, it is rather burdensome to implement a threadsafe javascript value reference counting.

In a conclusion, I didn't figure out a safe solution that node-addon-api could provide. Rather, I find that one of the core problems of the example above is that those callbacks API exposed by node-addon-api can not be non-copyable lambdas. If non-copyable lambdas fit in those APIs, Napi::References no need to be copyable in the case.

I've submitted #915 to illustrate the solution.

@legendecas legendecas removed the stale label Feb 26, 2021
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
allow non-copyable callbacks be finalizer parameters

Fixes: nodejs/node-addon-api#301
PR-URL: nodejs/node-addon-api#915
Reviewed-By: Michael Dawson <midawson@redhat.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
allow non-copyable callbacks be finalizer parameters

Fixes: nodejs/node-addon-api#301
PR-URL: nodejs/node-addon-api#915
Reviewed-By: Michael Dawson <midawson@redhat.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
allow non-copyable callbacks be finalizer parameters

Fixes: nodejs/node-addon-api#301
PR-URL: nodejs/node-addon-api#915
Reviewed-By: Michael Dawson <midawson@redhat.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
allow non-copyable callbacks be finalizer parameters

Fixes: nodejs/node-addon-api#301
PR-URL: nodejs/node-addon-api#915
Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants