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.