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

Context-aware add-ons? #567

Closed
rolftimmermans opened this issue Oct 16, 2019 · 22 comments
Closed

Context-aware add-ons? #567

rolftimmermans opened this issue Oct 16, 2019 · 22 comments
Labels

Comments

@rolftimmermans
Copy link
Contributor

rolftimmermans commented Oct 16, 2019

I am reading this https://nodejs.org/api/addons.html#addons_context_aware_addons and am left wondering if node-addon-api can help making context aware native modules.

There does not seem to be a way to initialise a context-aware module with node-addon-api currently.

Additionally it seems to me that it would be nice to provide a default wrapper for per-context global state (what would normally be easily accomplished with static variables).

Any thoughts?

@addaleax
Copy link
Member

Fwiw, all N-API addons are considered context-aware.

Additionally it seems to me that it would be nice to provide a default wrapper for per-context global state (what would normally be easily accomplished with static variables).

I think napi_set_instance_data might be what you want – it’s technically behind the NAPI_EXPERIMENTAL flag, but you can use it if you’re targeting the latest version of Node.js.

(The documentation for it is kind of misleading imo – Node.js currently does not support loading addons from multiple vm Contexts, so right now it’s not wrong to say that the data is associated with the current agent, but conceptually, the association is with the current module instantiation, which would be per-Context and not per-Agent.)

@rolftimmermans
Copy link
Contributor Author

It does seem like napi_set_instance_data/napi_get_instance_data will provide a good solution. So all that is required is a nice pattern to initialise a module, associate a C++ class with the instance data and support retrieving it from a given Napi::Env handle.

Is there any way to polyfill this for Node 10? Would be a shame to have to wait for Node 10 to be out of LTS for it to be generally usable for add-ons.

@mhdawson
Copy link
Member

@gabrielschulhof I think you implemented napi_set_instance_date/napi_get_instance_data. I also remember that we refactored the internal structure at some point. Do you remember if that was before/after 10.x as that might affect how easily those two functions can be supported in the 10.x line.

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Oct 17, 2019

@rolftimmermans there are several solutions to attaching data to an addon instance, in increasing order of correctness and convenience, but decreasing availability in terms of older Node.js versions:

  • Use napi_wrap() to attach data to exports to find out when to delete the per-instance data, and pass the data pointer to each binding created with Napi::Function::New(), which has a parameter void* data.
    This can be done with the earliest version of Node.js, but will not actually free the data until the version that ships with n-api: render finalizers as env cleanup hooks node#28428, because exports is never actually garbage-collected.
  • Use napi_add_env_cleanup_hook() and pass the data pointer to each binding created with Napi::Function::New(). This is also available down to v8.12.0, and will actually be called when the environment exits.
  • Use napi_set_instance_data() and napi_get_instance_data(). This is most convenient because you need not pass a void* data to each function you create, but this is only available as of v12.8.0.

I agree that we could use a nice abstraction on top of these mechanisms to render addons as instance factories.

@gabrielschulhof
Copy link
Contributor

whew ... I did several edits to my comment after I wrote it. Please do not read the emailed version 😀

@rolftimmermans
Copy link
Contributor Author

Thanks for clearing this up! Very helpful. I see that ObjectWrap<T>::DefineClass also takes a void* argument which means that together with Function::New it appears to solve this problem.

@rolftimmermans
Copy link
Contributor Author

So it turns out that napi_add_env_cleanup_hook() works really well for this use case, but there are more issues that are solved by nodejs/node#28428. Would be really nice if this fix can make it into Node 12 LTS.

@rolftimmermans
Copy link
Contributor Author

I have gone down the napi_add_env_cleanup_hook() route, but am now figuring out that it seems impossible to detect if the env that is being cleaned up is the main thread (at process exit) or a worker thread. Apart from some thread/agent-specific state, the module I'm working on also has some shared, thread-safe, truly global state that I want to tear down at process exit.

I guess It's just not possible right now to detect when the module is loaded/released from the main thread, correct?

I have attempted to use static destructors for this cleanup but it does not seem to work correctly on windows.

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Oct 21, 2019

@addaleax how safe is it to assume that the relationship between the environment and the thread is 1:1?

@rolftimmermans could you use thread-safe refcounting to decide when to tear down the per-process state? I'm fairly certain that the main thread is the last one to exit.

@devsnek
Copy link
Member

devsnek commented Oct 21, 2019

You can have multiple napi environments in a single thread, but you can't have multiple threads interacting with a napi environment.

@gabrielschulhof
Copy link
Contributor

@addaleax @devsnek In my previous question I was thinking about Node::Environments, actually, not napi_envs. Sorry about not being explicit!

@rolftimmermans
Copy link
Contributor Author

rolftimmermans commented Oct 22, 2019

Alright, the following is a summary of I have now, based on your comments. It might be useful for others.

This seems to work well for Node 10+, except that it appears nodejs/node#28428 won't be included in a stable version until Node 14, which means that so far any use of threads with native addons is giant memory leak (I haven't found a reliable workaround for this unfortunately – suggestions welcome!).

class GlobalState {
    // ... Globally shared, thread-safe state.
    // e.g. uint64_t FooBar;
    
public:
    static std::shared_ptr<GlobalState> Instance() {
        static std::mutex lock;
        static std::weak_ptr<GlobalState> shared;

        std::lock_guard<std::mutex> guard(lock);

        // Get an existing instance from the weak reference, if possible.
        if (auto instance = shared.lock()) {
            return instance;
        }

        // Create a new instance and keep a weak reference.
        // Global state will be cleaned up when last thread exits.
        auto instance = std::make_shared<ContextReaper>();
        shared = instance;
        return instance;
    }
};

class Module {
    // Pointer to shared global state
    std::shared_ptr<GlobalState> global_state = GlobalState::Instance();

    // ... Other per-module state
    // e.g.: Napi::FunctionReference foo_constructor;

public:
    GlobalState& GlobalState() {
        return *global_state;
    }

    Module(Napi::Object exports) {
        // Initialization per module, assigns constructors, etc.
        auto proto = {
            // ... InstanceMethod("foo", &Foo::foo),
        };
        
        // Pass module pointer as last argument.
        DefineClass(exports.Env(), "Foo", proto, this);
    }
};

class Foo : public Napi::ObjectWrap<Foo> {
    Module& module;

public:
    // Construct native object. `info.Data()` contains pointer to module.
    Foo(const Napi::CallbackInfo& info)
    : Napi::ObjectWrap<Foo>(info),
      module(*reinterpret_cast<Module*>(info.Data())) {
        // Access global state with e.g. `module.GlobalState().FooBar`
    }
}

NAPI_MODULE_INIT(/* env, exports */) {
    auto module = new Module(Napi::Object(env, exports));
    auto terminate = [](void* data) { delete reinterpret_cast<Module*>(data); };

    auto status = napi_add_env_cleanup_hook(env, terminate, module);
    assert(status == napi_ok);
    return exports;
}

In the future the module-wide state could be accessed via Napi::Environment if it wraps napi_set_instance_data/napi_get_instance_data, and that means the pointer to the module does not have to be passed during class definition, correct?

@addaleax
Copy link
Member

@addaleax how safe is it to assume that the relationship between the environment and the thread is 1:1?

@gabrielschulhof This is currently always true for the Node.js release binary/when used with Workers, but native addons or embedders don’t need to follow any rules regarding relationships between threads and Environment instances. I would avoid relying on any relationship of that kind as far as possible.

Apart from some thread/agent-specific state, the module I'm working on also has some shared, thread-safe, truly global state that I want to tear down at process exit.

@rolftimmermans If you are looking for “global” state that should be torn down when your addon is unloaded, then static data in your addon code should be the way to go.

This seems to work well for Node 10+, except that it appears nodejs/node#28428 won't be included in a stable version until Node 14

Why do you think it won’t be included into Node 13 and backported to earlier release lines? While I certainly think we should be cautious about that, I think this is what we should be doing.

which means that so far any use of threads with native addons is giant memory leak (I haven't found a reliable workaround for this unfortunately – suggestions welcome!).

Yeah, that’s just a really, really unfortunate fact in terms of N-API’s design. You should be able to use napi_add_env_cleanup_hook in order to emulate the behaviour introduced in nodejs/node#28428, but that’s going to be a lot of manual work unless node-addon-api implements it for all references on its own.

@rolftimmermans
Copy link
Contributor Author

Why do you think it won’t be included into Node 13 and backported to earlier release lines? While I certainly think we should be cautious about that, I think this is what we should be doing.

@addaleax It would be nice if it could be back-ported, definitely!

Yeah, that’s just a really, really unfortunate fact in terms of N-API’s design. You should be able to use napi_add_env_cleanup_hook in order to emulate the behaviour introduced in nodejs/node#28428, but that’s going to be a lot of manual work unless node-addon-api implements it for all references on its own.

Having it in node-addon-api would be ideal. Maybe with a compile time flag if it has any runtime overhead. If I have time, I might try to work on an implementation. Any tips how to approach this would be welcome!

@gabrielschulhof
Copy link
Contributor

@rolftimmermans nodejs/node#30070

@gabrielschulhof
Copy link
Contributor

@rolftimmermans I have a POC that does not use node-addon-api, but which can no doubt be adapted:

The boilerplate:

template <class AddonClass>
class AddonBase {
 public:
  virtual ~AddonBase() {
    while(methods != nullptr) {
      MethodData* next = methods->next;
      delete methods;
      methods = next;
    }
  }

  virtual napi_value Init(napi_env env, napi_value exports) {
    return exports;
  }

  static napi_value New(napi_env env, napi_value exports) {
    AddonClass* instance = new AddonClass();
    napi_set_instance_data(env,
                           instance,
                           Delete,
                           nullptr);
    return instance->Init(env, exports);
  }

  static void Delete(napi_env env, void* data, void* hint) {
    delete static_cast<AddonClass*>(data);
  }

  typedef napi_value(AddonClass::*MethodPointer)(napi_env env,
                                                 napi_callback_info info);

  struct MethodData {
   public:
    MethodData(MethodPointer cb, AddonClass* instance, MethodData* next):
      cb(cb), instance(instance), next(next) {}
    MethodPointer cb;
    AddonClass* instance;
    MethodData* next;
  };

  void AddMethod(napi_env env,
                 napi_value exports,
                 const char* name,
                 MethodPointer method) {
    napi_value result;
    methods = new MethodData(method, static_cast<AddonClass*>(this), methods);
    napi_create_function(env,
                         name,
                         NAPI_AUTO_LENGTH,
                         Dispatcher,
                         methods,
                         &result);
    napi_set_named_property(env, exports, name, result);
  }

 private:
  static napi_value Dispatcher(napi_env env, napi_callback_info info) {
    size_t argc = 0;
    void* raw_data;
    napi_get_cb_info(env, info, &argc, nullptr, nullptr, &raw_data);
    MethodData* mdata = static_cast<MethodData*>(raw_data);
    return ((mdata->instance)->*(mdata->cb))(env, info);
  }
  MethodData* methods = nullptr;
};

and its usage:

class MyAddon: public AddonBase<MyAddon> {
 public:
  napi_value Init(napi_env env, napi_value exports) override {
    // List methods to expose here.
    AddMethod(env, exports, "somethingUseful", &MyAddon::SomethingUseful);
    return exports;
  }

  // The binding is an instance method and so has access to the instance.
  napi_value SomethingUseful(napi_env env, napi_callback_info info) {
    napi_value result;
    napi_create_uint32(env, ++call_count, &result);
    return result;
  }
 private:
  size_t call_count = 0;
};

NAPI_MODULE_INIT() {
  return MyAddon::New(env, exports);
}

I'll check to see how best to integrate this with node-addon-api and its multiple ways of exposing bindings.

@Superlokkus
Copy link

If you are looking for “global” state that should be torn down when your addon is unloaded, then static data in your addon code should be the way to go.

I ran into some trouble when using static globals that are initialized in the before-main time on windows machines, at least while using cmakejs instead of gyp: cmake-js/cmake-js#205 , however static local variables are working fine, so I recommend to use those.

However the whole context aware addon problematic came to my attention just of today, so my bug might be related to my probale misunderstanding of this.

Correct me if I am wrong: Since the whole shared library is loaded only once, at the electron app seems to be just one node-process (but multiple envs/worker threads /agents), I could use per process globals like static local variables for whole process granularity, as long as I do in a thread safe matter, right? Only draw back, would be the missing clean up. But at least I could have a global USB Device context, and queue the commands issued by my JS code.

@addaleax
Copy link
Member

@Superlokkus Yes, everything you said makes sense 👍

@gabrielschulhof
Copy link
Contributor

@Superlokkus additionally, if you need per-addon-instance data, you can use napi_set_instance_data() and napi_get_instance_data() and you will get cleanup whenever an instance of your addon is unloaded. These are core N-APIs, but we're making them available in node-addon-api v3.0.0 (to be released soon) under Napi::Env::SetInstanceData() and Napi::Env::GetInstanceData().

node-addon-api/napi.h

Lines 192 to 204 in 9c9accf

template <typename T> T* GetInstanceData();
template <typename T> using Finalizer = void (*)(Env, T*);
template <typename T, Finalizer<T> fini = Env::DefaultFini<T>>
void SetInstanceData(T* data);
template <typename DataType, typename HintType>
using FinalizerWithHint = void (*)(Env, DataType*, HintType*);
template <typename DataType,
typename HintType,
FinalizerWithHint<DataType, HintType> fini =
Env::DefaultFiniWithHint<DataType, HintType>>
void SetInstanceData(DataType* data, HintType* hint);

Whoops ... no docs yet 😳

gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this issue Apr 26, 2020
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Re: nodejs#567 (comment)
gabrielschulhof pushed a commit to gabrielschulhof/node-addon-api that referenced this issue May 1, 2020
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Re: nodejs#567 (comment)
gabrielschulhof pushed a commit that referenced this issue May 4, 2020
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Re: #567 (comment)
PR-URL: #708
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@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.

@github-actions github-actions bot added the stale label Dec 16, 2020
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Re: nodejs/node-addon-api#567 (comment)
PR-URL: nodejs/node-addon-api#708
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Re: nodejs/node-addon-api#567 (comment)
PR-URL: nodejs/node-addon-api#708
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Re: nodejs/node-addon-api#567 (comment)
PR-URL: nodejs/node-addon-api#708
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Re: nodejs/node-addon-api#567 (comment)
PR-URL: nodejs/node-addon-api#708
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@vipin387
Copy link

vipin387 commented Apr 9, 2024

Dear All,
This thread i think deals with some what the issue we are facing currenty.
Our requirement is that using napi addon we are trying to load core-network shared library , we are attaching callback to core-network shared library ,whenever there is change in network status , when our callback is hit ,we want to call the JS layer from the addon.
Scenario 1.
JS-Layer-------------->napi addon--->calling apis of--->network core library
Getting Result at JS<---<Return result--------------------------------return result
This is running fine for us and we are able to get the response data in JS laye
Scenario 2.
JS-Layer-------------->napi addon--->registering cabllback -- --->network core library
<????????---------callback invoked----------------------------calling callback function
Here in Scenario 2. we are facing issue in getting the napi env and calling any JS function.

Any help would be highly appreciated..

@KevinEady
Copy link
Contributor

Hi @vipin387 ,

This thread is a little different than what features you need. You need to use thread-safe functions.

If you need further assistance, please open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants