New API to store per-addon-instance data

[gabrielschulhof]

Picking up from https://github.com/nodejs/node/pull/28464#issuecomment-508226398:

@addaleax:

@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.

[gabrielschulhof]

Also, @nodejs/n-api 🙂

[gabrielschulhof]

should be stored on the node::Environment and not on the per-addon napi_env.

Actually, we could store that shared data on the global like we're storing the napi_env now, and give napi_env a field like shared which would be initialized from GetEnv() or NewEnv() as it will likely be called.

[addaleax]

WDYT about the API below?

@gabrielschulhof That sounds good to me :+1:

should be stored on the node::Environment and not on the per-addon napi_env.

Actually, we could store that shared data on the global like we're storing the napi_env now, and give napi_env a field like shared which would be initialized from GetEnv() or NewEnv() as it will likely be called.

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_env object and represents that shared state.

[gabrielschulhof]

@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_env and get passed around, but it is the local component (napi_local_env?) that will be stored in CallbackBundles and will have a pointer to napi_env to then pass around to the addon. I'm also thinking about adding a pointer onto napi_env that points to the napi_local_env so 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]

@addaleax my idea is to have both a local component and a shared component such that they point to one another.

:+1:

Right now it looks like the shared component will still be called napi_env and get passed around, but it is the local component (napi_local_env?) that will be stored in CallbackBundles and will have a pointer to napi_env to then pass around to the addon.

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_env opaque pointer in the API) would be easier, and obviate the need for a pointer from the shared structure to the local one?

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,

Yes, I would say so.

right, because the life cycle of the local addon data, if allocated, will coincide with the life cycle of the environment?

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]

My first thought was also that the napi-env pointing to the local environment was what would make sense.