AsyncWorker structural defects and limitations

[ebickle]

The current design of napi::AsyncWorker has a number of defects and limitations with it's current design

Issues

Unsafe Self-Destructing Behavior

Managing object lifetimes across multiple threads is a challenging problem. In the case of AsyncWorker, the caller that instantiates a new AsyncWorker instance has no control over when the work will be completed. To work around this problem, the AsyncWorker class currently has a "self-destructing" behavior - the last step of the OnWorkComplete callback function causes the object to delete itself. (delete self;). This behavior is unsafe and causes a number of problems:

1. The AsyncWorker class does not know how the AsyncWorker memory was allocated. Imagine the following code as an example:
   void runWorkOnThread() { AsyncWorkerSubclass worker; worker.Queue(); }
   In this case, new was never called and the AsyncWorker (unfortunately) lives on the stack.

2. the work_complete callback executes asynchronously from the caller's lifetime of the object. If the caller holds a reference to the AsyncWorker and later calls Cancel() asynchronously from the main node.js event loop, a race condition will occur. Cancellation is always an asynchronous operation and must not fail in a dangerous manner when called after execution of the asynchronous work has already completed. Imagine this scenario:

   • The AsyncWorker is instantiated and queued, a reference to the AsyncWorker pointer is retained.
   • Call flow returns out of the addon and back to node.js.
   • Node.js executes other tasks on the event loop, one of which is the work_complete callback.
   • The AsyncWorker is deleted.
   • User code is executed on the Node.js event loop that calls an operation on the addon that holds the reference to the AsyncWorker and calls Cancel.
   • Invalid memory is referenced - boom.

Hard-coded Javascript Function Callbacks

The AsyncWorker currently takes a callback napi::Function and optional napi::Object receiver as required parameters. The intent behind the callback function is to provide an easy mechanism to automatically call a user-defined callback function outside of the addon, but this makes an incorrect assumption that the implementing addon will use a callback design pattern.

Consider a "legacy-free" addon that wishes to return a Promise for direct use with async and await. The addon would need to implement a Javascript function as a callback, wrap C++ code inside the Javascript Function that marshals their return value and/or errors, then have that C++ code signal a deferred.

The requirement for a "Javascript round-trip" to execute C++ code or implement Promises is a major headache.

No Suppose for Promises

As discussed above, there is no way to directly bridge an AsyncWorker to a Promise.

No Support for Error objects

The AsyncWorker currently only supports Javascript Error objects instantiated with a message; there is no way to return a "richer" Error object or different error type from within an AsyncWorker. In my own case, I want to return additional, important error information returned from the proprietary encryption engine I'm wrapping - there's no way to do this presently, even from within OnOK() or OnError(), which execute on the main Node.js event loop.

No Return Values

There is no mechanism to pass a return value out of the AsyncWorker, whether to the callback function or a Promise.

Unsafe Access to Napi::Env

The class contains a public reference to a napi_env retrieved from the callback function passed to the constructor. While it's ultimately up to implementers to write their code correctly, the public Env() function is a dangerous temptation - if used from within the Execute() function, multiple threads will be unsafely using the same v8 Isolate.

Resolution

I've been exploring a number of potential solutions to the problems listed above, but I haven't solved all of the issues in a single solution I can pitch to the node-addon-api community. The intent behind this issue is to create a single place to list all of the potential problems with the current type and to keep track of the various design options and discussion for resolving the defects and limitations.

Potential ideas I've been exploring include:

• Removing the Env() function from the AsyncWorker type and modifying OnOK() and OnError() to take a Napi::Env as a parameter instead. .
• Changing void Queue() to static void Queue(AsyncWorker* worker); to force it to be a pointer.
• Removing the self-deleting behavior in favor of the caller managing lifetime. The idea would be to have a higher level class/construct that addon implementers would use, while the AsyncWorker would mainly be used internally for management of the napi_async_work resource.
• Remove the callback function from the type and separate out any callback and/or promise behavior to subclasses.
• Have Queue() return an object with a cancellation token (Cancel function) and methods to bind to a Promise or a Callback function.
• Replace OnError() and OnOK() with a virtual napi_value OnComplete(Napi::Env env) that could return a value back from the asyncronous operation. Note this still has the problem of getting the value or error safely out of the execute function.
• Add reference counting internally to avoid thread-lifetime issues with napi_async_work.
• Add a higher level construct that allows the use of async work without subclassing. For example, something that worked similarly to std::async(); something like a Napi::WorkerPool::Run(Callable) that could take a lambda function, regular function, or callable type.
• Exploring options of bridging C++'s async types (std:future, std::promise, etc) with AsyncWorker. Probably not feasible, but newer versions of C++ are adding support for things like Future.then().

No slam-dunk options yet - just quite a few ugly trade-offs. The idea of a higher level type that takes a Callable (lambda/etc) is very tempting, but the only clean way to do that and still keep our sanity is to use std::invoke. That creates a dependency on C++17 - in the case of Windows, Visual Studio 2015 or so.

[ebickle]

Low Level Implementation - Napi::AsyncWork

To solve some of the issues listed above, a low-level class that handles the lifetime of the napi_async_work is needed. Currently AsyncWorker creating and deleting the napi_async_work, but the design of AsyncWorker causes its lifetime to be shared across multiple threads in an unsafe manner.

My proposal is to create a new AsyncWork class that handles the creation and deletion of napi_async_work and wraps the other related async_work functions (queue and cancel). Instead of using virtual functions and having consumers subclass, the new AsyncWork is designed to be self-contained.

It's expected that most consumers won't use AsyncWork directly. A separate, higher-level type (TBD) inside of node-addon-api will act as the main interface for consumers and provide support for Javascript callback functions, Promises, and/or C++ Lambdas.

Proposed class definition

  class AsyncWork {
  public:
    typedef void (*AsyncWorkExecuteCallback)(void* data);
    typedef void (*AsyncWorkCompleteCallback)(Napi::Env env, void* data);

    explicit AsyncWork(Napi::Env env, 
                       AsyncWorkExecuteCallback execute,
                       AsyncWorkCompleteCallback complete,
                       void* data = nullptr);
    ~AsyncWork();

    // Async work can be moved but cannot be copied.
    AsyncWork(AsyncWork&& other);
    AsyncWork& operator =(AsyncWork&& other);
    AsyncWork(const AsyncWork&) = delete;
    AsyncWork& operator =(AsyncWork&) = delete;

    operator napi_async_work() const;

    Napi::Env Env() const;    

    void Queue();
    void Cancel();

  private:
    static void OnExecute(napi_env env, void* data);
    static void OnComplete(napi_env env, napi_status status, void* data);

    napi_env _env;
    napi_async_work _work;  
    AsyncWorkExecuteCallback _execute;
    AsyncWorkCompleteCallback _complete;
    void* _data;
  };

Implementation details

• Designed to work as both a reference and a pointer.
• Object lifetime entirely controlled by consumer; no "self-destructing" behavior.
• Non-copyable object. Shared references to the async work will be handled by consumers.
• Not designed to be subclassed.

Questions

• No other API inside of node-addon-api passes or returns a napi_status. Instead of passing it down to the complete callback as we do today, should it be automatically converted to an Error as part of the callback process? In other words, if not cancelled or ok immediately perform a NAPI_THROW_IF_FAILED before the supplied callback is executed?
• Callbacks are defined as typedefs instead of "Callable" template types. The intent is to avoid requiring the entire AsyncWork type to be a template - is this the correct tradeoff? Some other types that have C++ callbacks from Javascript use .

[mhdawson]

@nodejs/n-api please review and comment.

[chad3814]

@ebickle, sort of related, I illegally (I guess) tried to use Env() within Execute(), and dumped core. How should I do that? For example I want to create a new Buffer object, and call a js function with it.

[gabrielschulhof]

@ebickle you could simply allocate memory inside Execute(), pass it to the Complete() callback and then create an external buffer in the Complete() callback.

…

On Fri, Jun 8, 2018 at 5:23 PM, chad3814 ***@***.***> wrote: @ebickle <https://github.com/ebickle>, sort of related, I illegally (I guess) tried to use Env() within Execute(), and dumped core. How should I do that? For example I want to create a new Buffer object, and call a js function with it. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#231 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0SmPgx5j5eYfIOIcQGCQW0r2MXz1ks5t6utWgaJpZM4ScYXw> .

[mhdawson]

@chad3814, you should not be interacting with JavaScript (either through node-addon-api calls or otherwise) in Execute() since it does not run on the main event loop. We are improving the documentation to clarify that.

[mhdawson]

@ebickle sorry for not having gotten to this yet. Our current focus is completing the docs for the existing classes in node-addon-api and then we can engage on future improvements.

[rivertam]

Just to clarify, is using Promises with an AsyncWorker valid? I'm not sure exactly how to do this. It's okay to me if I have to resolve the Promise in the callback, but I'd rather not have to write a JS wrapper outside of C++ to wrap the value in a Promise.

Here's what I'm trying right now:

auto deferred = Napi::Reference::New(Napi::Promise::Deferred::New(info.Env()), 1);

Napi::Function cb = Napi::Function::New(
  info.Env(),
  [deferred=Napi::Reference::New(deferred.Value(), 1)] (const Napi::CallbackInfo & info) {
    deferred->Resolve(Napi::String::New(info.Env(), "hello!"));
    deferred.Unref();
  },
  "onStartComplete"
);

(new StartWorker(cb))->Queue();

It is very much unclear to me if I'm going with the right strategy with references, Promises, and AsyncWorker, so any advice is very much appreciated. I'd like to modify the Promise/AsyncWorker docs to reflect this usecase which I speculate is a fairly common one.

Edit: It looks like I can just copy the Napi::Promise::Deferred, so I never needed to make a reference. Everything else worked properly.

[chad3814]

@mhdawson so I ended up dropping n-api/node-addon-api and just doing everything in straight v8/node. This is unfortunate. I'd like to be able to use Napi::AsyncWorker like this gist: https://gist.github.com/chad3814/50d75d4f13054dc47de8c897f303580e

[mhdawson]

@chad3814 I'm not sure I understand why you could not have had the content of WorkAsyncComplete() in OnOK, and the content of WorkAsync in Execute() ?

[chad3814]

Thanks @mhdawson, I'm not entirely sure how to do the emit() using node-addon-api. How do I get an Env, etc..

[rivertam]

I'm running into a memory leak due to this complication (though the underlying issue is probably my lack of understanding of object lifecycle in V8 as well as some other stuff).

I'm trying to do a loop similar to setInterval in C++. I believe the proper way to do this is:

// There's a bit of an issue in this code alone. Assume there are no linker errors
// and anywhere there's a stupid compiler issue I probably just forgot some boilerplate
class Worker : public Napi::AsyncWorker {
public:
  void Execute() override {
    // Body
  }

  void OnOK() override {
    queueLoop(this->Env());
  }

  void OnError(const Napi::Error & e) override {
    queueLoop(this->Env());
  }
}

void queueLoop(Napi::Env env) {
  Napi::HandleScope scope(env);
  // I don't actually need a callback for business logic
  Napi::Function cb = Napi::Function::New(
    env, [] (const Napi::CallbackInfo & info) {}, "onLoopComplete"
  );

  (new Worker(cb))->Queue();
}

Now, this is actually working properly in terms of looping and such. 🎉

However, while the Worker gets destroyed on every loop (the destructor is called), the callback function cb just isn't. After a minute or two of very fast looping (Execute takes a variable amount of time, but that's irrelevant), the heap ends up at about a gigabyte with hundreds of thousands of copies of this callback not getting garbage collected.

1. How can I make sure they get garbage collected? I thought the HandleScope might solve the issue, but otherwise I just don't know.
2. If AsyncWorker didn't require a callback, I wouldn't be running into this issue.

I tried replacing the queueLoop calls in the OnOK and OnError with just this->Queue(), but of course this resulted in a segfault.

edit: I have a solution for my particular need, but I don't think it addresses the multiple core issues that I'm perceiving. My solution is as follows:

std::optional<FunctionReference> emptyCallback;

// ...

void queueLoop(Napi::Env env) {
  Napi::HandleScope scope(env);
  if (!emptyCallback) {
    Napi::Function cb = Napi::Function::New(
      env, [] (const Napi::CallbackInfo & info) {}, "onLoopComplete"
    );

    emptyCallback = Napi::Persistent(cb);
    emptyCallback->SuppressDestruct();
  }

  (new Worker(emptyCallback->Value()))->Queue();
}

[mhdawson]

@rivertam does it actually run out of memory, or does the heap just grow to that size and then keep running staying consistently at that value?

Trying to understand if there is a leak (in which case the heap should keep growing) or just that the heap grows to a larger than expected size.

[rivertam]

@mhdawson Not sure what you mean. There was 100% a leak. Any more than one instance of that callback existing at a time (pending garbage collection) is a leak in my mind. In my case, once the Worker is destroyed, the callback should have been marked as safe to GC. We were looking at nearly a million instances of the function after about 50 seconds despite no two Workers ever existing at the same time.

After some time (between 1 and 2 minutes), the garbage collection process would crash the program with a heap allocation error. I'm not at my work computer right now, but the error said something along the lines of the heap running out of memory.

[mhdawson]

@rivertam Ok, that answers my question as to whether you actually ran out of memory or just had a large heap. Which version of Node.js were you using and is the code that showed the problem somewhere we can easily install/run?

[rivertam]

node version: 10.2.1

It's not and I don't have the time to make a minimal repro repo, unfortunately, but the relevant code was most certainly the first code in my most recent comment. If you feel as though you absolutely need a minimal repo to reproduce, remind me some time next week and I'll try to make it, but I think it's very clear what I was doing based on the first code sample.

[gabrielschulhof]

@rivertam I'm fairly certain that V8 never garbage-collects JavaScript functions which run native code. Thus, there will always be a leak if you create such functions in a loop.

I can think of two solutions:

1. Make the callback to Worker optional
2. Create the function that gets passed to Worker in JavaScript

[mhdawson]

I'll take the action to add some additional documentation to the N-API/node-addon-api docs to highlight that those functions won't be collected.

[mhdawson]

@hashseed can you confirm that V8 does not garbage collect JavaScript functions which run native code?

[gabrielschulhof]

@rivertam https://gist.github.com/gabrielschulhof/43abe2c9686a1c0b19fcf4784fa796ae has an example that illustrates the lack of GC-ing, even without N-API.

[hashseed]

Instances created from function templates are cached in an instantiation cache. The assumption is that we have a fixed number of bindings, so this will not cause a leak.

I think what you want to do is to use v8::Function::New instead of creating a v8::FunctionTemplate and instantiating it. Function bindings created this way do not get cached in the instantiation cache.

[gabrielschulhof]

v8::Function::New is indeed the way to go.

[gabrielschulhof]

This also means that we need to make N-API use v8::Function::New whenever it dynamically creates a function.

[gabrielschulhof]

@mhdawson I just submitted nodejs/node#21688 which switches N-API to using v8::Function::New(), and thus, to properly gc-ing functions, in all cases except napi_define_class(), because there we need to use signatures to ensure that the prototype methods are called with the proper context which, in turn, requires that we use a function template.

Once that lands, we'll have to once more sync up the implementation of N-API provided by node-addon-api with the implementation provided by Node.js.

[rivertam]

@gabrielschulhof Thanks for the advice. The static version that I'm working with right now seems like the optimal thing in all cases, though less ergonomic.

It would be nice if N-API could expose something along the lines of setInterval and setTimeout so you wouldn't have to manage this yourself. I was a little surprised I had to do a custom recursion. I ended up writing probably 50+ lines just to emulate this behavior, probably suboptimally.

[mhdawson]

@gabrielschulhof I'm pretty sure but just to be 100% sure, nodejs/node#21688 just makes things better (in terms of memory use) and we only need to backport to get getter memory use in the older version as well right?

[mhdawson]

@gabrielschulhof did you also check if we need changes in node-addon-api to ensure that Functions are collected when possible when using node-addon-api?

[gabrielschulhof]

@mhdawson we definitely need changes to node-addon-api to make it possible to remove the data it associates with callbacks.

Whenever we change the function signature of the binding, we're essentially adding another layer of indirection where

1. We pass the same function with the old signature on behalf of all bindings with the new signature, and
2. We pass some heap data along which contains the address of the binding with the new signature as well as the data to pass to it.

This is unavoidable, unless we manage to shuffle the data type declarations such that the compiler recognizes a function with the new signature as being equivalent to a function with the old signature. I doubt this is possible.

So, the only choice that does not involve changes to N-API and which allows us to destroy the data associated with a function created by node-addon-api is to

1. napi_wrap() each napi_value representing a function we've created using node-addon-api, and provide a finalizer,
2. modify node-addon-api's implementation of the object wrap's unwrap portion to recognize data associated with a function, if present, and to tack on the data the user wishes to wrap in addition to the function data.

This approach has the limitation whereby a function wrapped using node-addon-api cannot be unwrapped with plain napi_unwrap() because the resulting data is not merely the data that was given by the addon maintainer, but the data added by node-addon-api when the function was created followed by the pointer to the data the addon maintainer provided.

Another undesirable(?) effect of this approach is that it would take node-addon-api beyond mere syntactic sugar by locking the user into using it for both wrapping and unwrapping.

The truth is though that node-addon-api is more than syntactic sugar even today, because it allocates this dynamic data which then needs to be freed.

Unless we can find some template syntax magic that explains to the compiler that

Napi::Value SomeFunction(const Napi::CallbackInfo& info) {
// Closing brace intentionally left out

is to be converted to

napi_value SomeFunction(napi_env env, napi_callback_info cb_info) {
Napi::CallbackInfo info(env, cb_info);
// Closing brace intentionally left out

the only clean choice seems to be to abandon the sugar for function, accessor, and class static callback signatures.

Class constructor and prototype methods are safe so far, but, technically, other JS engines may garbage-collect entire classes when they go out of scope.

[mhdawson]

@gabrielschulhof lets discuss this in the next N-API team meeting.

[mhdawson]

@KevinEady is going to work on the changes to AsyncWorker that we discussed above.

[mhdawson]

@KevinEady landed the changes outlined above for Return Values, so only issue not addressed would be the one related to Error objects but we think there is a way to handled that which was described as:

it should be possible to decorate the Error passed to OnError. So we believe what is being asked for is possible by adding extra info to the instance off the main thread and then using that information to decorate the Error passed in to OnError. Is there something we've missed that means this is not possible?

@ebickle does what we've described work for your use case?

[mhdawson]

Going to close. @ebickle please re-open if you believe there is still something we need to address.

[greg9504]

Here is my adaption of your class, just some minor refactoring and namespace clean up:

If you run across this thread because you are looking how to use a Promise with AsyncWorker and copy the PromiseWorker class from @Superlokkus there is a small addition that's needed to prevent an exception on exit.
The fake callback needs to have fake_callback.SuppressDestruct(); called. See SuppressDestruct.

My modified class

#include <napi.h>

class PromiseWorker : public Napi::AsyncWorker {
public:
    PromiseWorker(Napi::Promise::Deferred const &d, const char* resource_name) : AsyncWorker(get_fake_callback(d.Env()).Value(), resource_name), deferred(d) {}
    PromiseWorker(Napi::Promise::Deferred const &d) : AsyncWorker(get_fake_callback(d.Env()).Value()), deferred(d) {}

    virtual void Resolve(Napi::Promise::Deferred const &deferred) = 0;

    void OnOK() override {
        Resolve(deferred);
    }

    void OnError(Napi::Error const &error) override {
        deferred.Reject(error.Value());
    }

private:
    static Napi::Value noop(Napi::CallbackInfo const &info) {
        return info.Env().Undefined();
    }

    Napi::Reference<Napi::Function> const &get_fake_callback(Napi::Env const &env) {
        static Napi::Reference<Napi::Function> fake_callback
                = Napi::Reference<Napi::Function>::New(Napi::Function::New(env, noop), 1);
        fake_callback.SuppressDestruct();

        return fake_callback;
    }

    Napi::Promise::Deferred deferred;
};

[nkallen]

The static in the above PromiseWorker code is unsafe

Also, you no longer need a fake_callback

class PromiseWorker : public Napi::AsyncWorker {
public:
    PromiseWorker(Napi::Promise::Deferred const &d, const char* resource_name) : AsyncWorker(d.Env(), resource_name), deferred(d) {}
    PromiseWorker(Napi::Promise::Deferred const &d) : AsyncWorker(d.Env()), deferred(d) {}

    virtual void Resolve(Napi::Promise::Deferred const &deferred) = 0;

    void OnOK() override {
        Resolve(deferred);
    }

    void OnError(Napi::Error const &error) override {
        Reject(deferred, error);
    }

    virtual void Reject(Napi::Promise::Deferred const &deferred, Napi::Error const &error) = 0;

private:
    Napi::Promise::Deferred deferred;
};

[greg9504]

@nkallen
Could you elaborate on your statements?
What version of napi?
Thanks

[nkallen]

@greg9504 if you want to use your add-on in a web worker or in modern electron, it needs to be context aware which means it cannot use anything static