ebickle
commented
6 years ago 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
chad3814
gabrielschulhof
rivertam
hashseed
DaAitch
jaubourg
Superlokkus
KevinEady
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: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,newwas never called and the AsyncWorker (unfortunately) lives on the stack.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:
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:
void Queue()tostatic void Queue(AsyncWorker* worker);to force it to be a pointer.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.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.