Repository navigation
New API to store per-addon-instance data #378
Description
Picking up from nodejs/node#28464 (comment):
@gabrielschulhof It’s not safe to run JS at that point, but otherwise I think this should work. And sure, it’s possible to do this, it’s just not very ergonomical at this point (imo).
You're right. It's not ergonomical. WDYT about the API below?
NAPI_EXTERN napi_status napi_set_module_data(napi_env env,
void* data,
napi_finalizer finalize_cb,
void* finalize_hint);
NAPI_EXTERN napi_status napi_get_module_data(napi_env env, void** data);We are already using can_call_into_js() to determine whether it's safe to call into JS, so I think we can offer napi_env in the cleanup hook, and thus we can render it as a napi_finalize.
This API would have the added advantage that bindings need not call napi_get_cb_info() to retrieve their void* data, because napi_get_cb_info() is fairly heavy.
Also, if we switch to a per-module napi_env, that's actually what we should've had all along, but any ObjectTemplates we may wish to store in the future (I hope there will be none) should be stored on the node::Environment and not on the per-addon napi_env.
Activity
gabrielschulhof commentedon Jul 6, 2019
Also, @nodejs/n-api 🙂
gabrielschulhof commentedon Jul 6, 2019
Actually, we could store that shared data on the global like we're storing the
napi_envnow, and givenapi_enva field likesharedwhich would be initialized fromGetEnv()orNewEnv()as it will likely be called.addaleax commentedon Jul 6, 2019
@gabrielschulhof That sounds good to me 👍
I’m not sure what your plans are for implementing per-module
napi_env, but I’d assume that it would still point to a struct that works like the current (per-Context)napi_envobject and represents that shared state.gabrielschulhof commentedon Jul 6, 2019
@addaleax my idea is to have both a local component and a shared component such that they point to one another. Right now it looks like the shared component will still be called
napi_envand get passed around, but it is the local component (napi_local_env?) that will be stored inCallbackBundles and will have a pointer tonapi_envto then pass around to the addon. I'm also thinking about adding a pointer ontonapi_envthat points to thenapi_local_envso that APIs involving the local env can have access to the local info. This pointer would be set to the local env before entering the addon and would be NULL-ed out before returning to JS.I guess from your earlier comment regarding the fact that the ECMAScript spec does nowadays outline the concept of an environment that comes and goes (an "Agent" IIUC), it would make sense to add these functions to
js_native_api.h, right, because the life cycle of the local addon data, if allocated, will coincide with the life cycle of the environment?addaleax commentedon Jul 7, 2019
👍
Can you explain why? On first thought it sounds like passing a pointer to the local environment (and letting that fill the role of the
napi_envopaque pointer in the API) would be easier, and obviate the need for a pointer from the shared structure to the local one?Yes, I would say so.
I could see an implementation deciding to unload addons e.g. when all of its exposed functions are garbage-collected. But I don’t think that this is relevant for the decision of where it goes.
mhdawson commentedon Jul 8, 2019
My first thought was also that the
napi-envpointing to the local environment was what would make sense.n-api: add APIs for per-instance state management
n-api: add APIs for per-instance state management
Please ignore this msg (#1)
n-api: add APIs for per-instance state management
n-api: add APIs for per-instance state management
n-api: add APIs for per-instance state management