node icon indicating copy to clipboard operation
node copied to clipboard

NAPI: Add Symbols invisible to getOwnPropertySymbols

Open Kangz opened this issue 2 years ago • 13 comments

What is the problem this feature will solve?

This is in the context of implementing dawn.node which is a Node module exposing the WebGPU API by translating JS calls to calls to Dawn, the library that Chromium uses to implement WebGPU.

The GPUDevice inherits from EventTarget so we need to implement a reasonable version of EventTarget in dawn.node. This requires keeping a list of callbacks to call them in dispatchEvent. Because Napi::Value are only valid during the scope of a call by default, the C++ implementation must keep a Napi::Reference to them so that the callback values stay valid between the call to addEventListener and dispatchEvent. However this could lead to reference cycles that prevent objects from being collected: the GPUDevice owns a CppEventTargetImplementation that contains Napi::Reference<Napi::Function> that's a closure referencing the GPUDevice. In this case the GPUDevice can never be collected.

What is the feature you are proposing to solve the problem?

An alternative is to hide the storage of the EventTarget values in the JS object graph by storing the data in an object keyed with a symbol only known by the C++ code. Essentiallydevice[SymbolKnownByCpp] = {eventTargetStuff}. However this symbol would still be discoverable with getOwnPropertySymbols so the encapsulation is not perfect (even with enumerable: false).

Could we have a way to make symbol properties hidden from getOwnPropertySymbols? Looking at the v8 and SpiderMonkey headers, both engines don't seem to have support for this, but maybe it could be added?

What alternatives have you considered?

I've tried keeping a Napi::Reference in my code, but this will lead to leaks.

Another possibility would be to add a tracing API to node that integrates with the marking phase of GCs, similar to what's in Blink with the Trace methods, but this seems like a MUCH harder problem and very engine dependent.

Kangz avatar Dec 06 '22 13:12 Kangz

Looking at the v8 and SpiderMonkey headers, both engines don't seem to have support for this, but maybe it could be added?

Node.js core has used v8::Private to hide such symbols. IIUC, it doesn't seem to exist in Node-APIs since it isn't a JavaScript feature.

daeyeon avatar Dec 07 '22 02:12 daeyeon

/cc @nodejs/node-api Can we allow this kind of feature on Node APIs? It seems like a use case worth considering.

daeyeon avatar Dec 07 '22 02:12 daeyeon

Is support for JS engines other than V8 still a design constraint for n-api? Because I don't think other engines have a similar feature.

bnoordhuis avatar Dec 07 '22 09:12 bnoordhuis

Is support for JS engines other than V8 still a design constraint for n-api? Because I don't think other engines have a similar feature.

This is still a documented constraint that we should stick to when adding new node-api functions. These rules allow us to avoid introducing node-api that is vulnerable to specific JS engine implementation-defined features without ABI guarantees. Notably, we have node-api implementations on other JS engines available too:

  • https://github.com/oven-sh/bun#node-api-napi
  • https://github.com/jerryscript-project/iotjs/tree/master/src/napi
  • More at https://github.com/nodejs/abi-stable-node/blob/doc/node-api-engine-bindings.md#node-api-bindings-for-other-runtimes.

legendecas avatar Dec 09 '22 06:12 legendecas

We discussed this in the 9 Dec 2022 Node-API meeting. @vmoroz will take a look at the possibility of using private symbols in Hermes engine so we can get a better understanding of support of this feature in other JavaScript engines.

KevinEady avatar Dec 09 '22 16:12 KevinEady

@Kangz If you want to have some fun in the short term, and maybe validate that private symbols as a concept will work for you, you can do something like this (not tested but should be more or less the right path i think...)

Napi::Value MakeSpookySymbol(Napi::Env env) {
  Local<Private> p = Private::New(Isolate::GetCurrent()); // i think you can extract isolate from `env` as well but i don't feel like digging through the internals to remember how this works
  Local<Value> sym = *reinterpret_cast<Local<Value>*>(&p);
  napi_value n = reinterpret_cast<napi_value>(*sym);
  return Napi::Value(env, n);
}

Also, you could consider using WeakMap, which has identical semantics to using this private symbol api, without relying on engine weirdness.

devsnek avatar Dec 11 '22 02:12 devsnek

Thanks all for the discussion!and progress so far, this is much more than I expected :)

Kangz avatar Dec 12 '22 09:12 Kangz

As for the Hermes JavaScript engine, they recently added a feature to associate native state with an Object. Comparing with V8, it means that Hermes has only a single predefined private property.

vmoroz avatar Dec 13 '22 03:12 vmoroz

@Kangz, would it be possible to use std::weak_ptr to break the reference loop? The std::weak_ptr is often used inside of the lambda capture. The napi_ref can also be used as a weak reference - it becomes a weak reference when the ref count is zero.

vmoroz avatar Dec 13 '22 04:12 vmoroz

This is still a documented constraint that we should stick to when adding new node-api functions. These rules allow us to avoid introducing node-api that is vulnerable to specific JS engine implementation-defined features without ABI guarantees. Notably, we have node-api implementations on other JS engines available too:

Speaking for bun, it is straightforward to add symbols invisible to getOwnPropertySymbols

Jarred-Sumner avatar Dec 22 '22 07:12 Jarred-Sumner

Could we have a way to make symbol properties hidden from getOwnPropertySymbols? Looking at the v8 and SpiderMonkey headers, both engines don't seem to have support for this, but maybe it could be added?

The "common" way would be to use a WeakMap to hold the references instead of a symbol on the object.

So instead of eventTarget[kBackingStorage] you would have backingStorageMap.get(eventTarget).

benjamingr avatar Dec 22 '22 12:12 benjamingr

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

github-actions[bot] avatar Jun 21 '23 01:06 github-actions[bot]

I think this can be solved with ObjectWrap. The Napi::ObjectWrap subclass contains a linked list of Napi::FunctionReference objects, each of which is a weak reference. A finalizer is added to each Napi::FunctionReference such that it removes the linked list item from the list when the Napi::FunctionReference is collected. That way, there is no reference loop, and no private properties are needed. Behind the scenes, ObjectWrap is implemented using a private property anyway, so this would piggy-back on that.

gabrielschulhof avatar Jun 21 '23 05:06 gabrielschulhof

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

github-actions[bot] avatar Dec 19 '23 01:12 github-actions[bot]

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

github-actions[bot] avatar Jan 18 '24 01:01 github-actions[bot]