Repository navigation
Napi::Reference not copyable, but semantically could be copyable #301
Description
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
Activity
mhdawson commentedon Jul 18, 2018
@nodejs/abi-stable-node please comment.
jasongin commentedon Jul 23, 2018
Napi::Referenceis equivalent tov8::Persistent(which may be "weak" or not). By defaultv8::Persistentis not copyable, though that restriction may be changed using a traits parameter. SeeCopyablePersistentTraitsin 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::Referencewith that more conservative restriction, with the thought that we can loosen the restriction later if necessary, either via similar traits or some other API.holder: implement holder mechanism
DaAitch commentedon 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
Holderimpl provided here wildoak@90294e4 .The idea is to take a
Napi::Valueto where you need it, e.g. as capture in lambdas.What do you think about it?
DaAitch commentedon 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::Referenceshould be whatv8::Persistentis, 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
Referenceis like ashared_ptr. They should all live on the stack. When they're destructed, ref count is decremented.So they are/have:
napi_ref_*)napi_unref_*)napi_refvalue, nonapi_*calls)napi_ref_*)src: implement `Napi::SharedValueReference<T>`
DaAitch commentedon Dec 27, 2018
Did another implementation on this as
Napi::SharedValueReference<T>.pro:
contra:
napi_*_reference_*API callsNow this is possible:
github-actions commentedon Dec 18, 2020
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 commentedon Jan 15, 2021
@legendecas will take a look and lead the discussion in a future meeting.
mhdawson commentedon Feb 5, 2021
@legendecas still looking at this one.
legendecas commentedon 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.
src: allow references to be copyable in APIs
src: allow references to be copyable in APIs
src: allow references to be copyable in APIs
src: allow references to be copyable in APIs