From ad157080d81c8768bb10c6e0aaa4807f4eb53993 Mon Sep 17 00:00:00 2001 From: John French Date: Wed, 6 Nov 2019 20:01:48 +0800 Subject: [PATCH] src: make OnWorkComplete and OnExecute override-able No breaking changes on existing code were expected. All existing tests shall pass without any touch. Changes on declaration: - Added `Napi::AsyncWorker::OnWorkComplete`. - Added `Napi::AsyncWorker::OnExecute`. PR-URL: https://github.com/nodejs/node-addon-api/pull/589 Reviewed-By: Gabriel Schulhof Reviewed-By: Michael Dawson --- doc/async_worker.md | 29 ++++++++++++++++++++ napi-inl.h | 66 ++++++++++++++++++++++++++------------------- napi.h | 55 +++++++++++++++++++------------------ 3 files changed, 96 insertions(+), 54 deletions(-) diff --git a/doc/async_worker.md b/doc/async_worker.md index 88668af..2573cd2 100644 --- a/doc/async_worker.md +++ b/doc/async_worker.md @@ -136,6 +136,35 @@ class was created, passing in the error as the first parameter. virtual void Napi::AsyncWorker::OnError(const Napi::Error& e); ``` +### OnWorkComplete + +This method is invoked after the work has completed on JavaScript thread. +The default implementation of this method checks the status of the work and +tries to dispatch the result to `Napi::AsyncWorker::OnOk` or `Napi::AsyncWorker::Error` +if the work has committed an error. If the work was cancelled, neither +`Napi::AsyncWorker::OnOk` nor `Napi::AsyncWorker::Error` will be invoked. +After the result is dispatched, the default implementation will call into +`Napi::AsyncWorker::Destroy` if `SuppressDestruct()` was not called. + +```cpp +virtual void OnWorkComplete(Napi::Env env, napi_status status); +``` + +### OnExecute + +This method is invoked immediately on the work thread when scheduled. +The default implementation of this method just calls the `Napi::AsyncWorker::Execute` +and handles exceptions if cpp exceptions were enabled. + +The `OnExecute` method receives an `napi_env` argument. However, the `napi_env` +must NOT be used within this method, as it does not run on the JavaScript +thread and must not run any method that would cause JavaScript to run. In +practice, this means that almost any use of `napi_env` will be incorrect. + +```cpp +virtual void OnExecute(Napi::Env env); +``` + ### Destroy This method is invoked when the instance must be deallocated. If diff --git a/napi-inl.h b/napi-inl.h index 5367a0b..c9fa638 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -4120,8 +4120,8 @@ inline AsyncWorker::AsyncWorker(const Object& receiver, _env, resource_name, NAPI_AUTO_LENGTH, &resource_id); NAPI_THROW_IF_FAILED_VOID(_env, status); - status = napi_create_async_work(_env, resource, resource_id, OnExecute, - OnWorkComplete, this, &_work); + status = napi_create_async_work(_env, resource, resource_id, OnAsyncWorkExecute, + OnAsyncWorkComplete, this, &_work); NAPI_THROW_IF_FAILED_VOID(_env, status); } @@ -4146,8 +4146,8 @@ inline AsyncWorker::AsyncWorker(Napi::Env env, _env, resource_name, NAPI_AUTO_LENGTH, &resource_id); NAPI_THROW_IF_FAILED_VOID(_env, status); - status = napi_create_async_work(_env, resource, resource_id, OnExecute, - OnWorkComplete, this, &_work); + status = napi_create_async_work(_env, resource, resource_id, OnAsyncWorkExecute, + OnAsyncWorkComplete, this, &_work); NAPI_THROW_IF_FAILED_VOID(_env, status); } @@ -4234,40 +4234,51 @@ inline void AsyncWorker::SetError(const std::string& error) { inline std::vector AsyncWorker::GetResult(Napi::Env /*env*/) { return {}; } +// The OnAsyncWorkExecute method receives an napi_env argument. However, do NOT +// use it within this method, as it does not run on the JavaScript thread and +// must not run any method that would cause JavaScript to run. In practice, +// this means that almost any use of napi_env will be incorrect. +inline void AsyncWorker::OnAsyncWorkExecute(napi_env env, void* asyncworker) { + AsyncWorker* self = static_cast(asyncworker); + self->OnExecute(env); +} // The OnExecute method receives an napi_env argument. However, do NOT -// use it within this method, as it does not run on the main thread and must -// not run any method that would cause JavaScript to run. In practice, this -// means that almost any use of napi_env will be incorrect. -inline void AsyncWorker::OnExecute(napi_env /*DO_NOT_USE*/, void* this_pointer) { - AsyncWorker* self = static_cast(this_pointer); +// use it within this method, as it does not run on the JavaScript thread and +// must not run any method that would cause JavaScript to run. In practice, +// this means that almost any use of napi_env will be incorrect. +inline void AsyncWorker::OnExecute(Napi::Env /*DO_NOT_USE*/) { #ifdef NAPI_CPP_EXCEPTIONS try { - self->Execute(); + Execute(); } catch (const std::exception& e) { - self->SetError(e.what()); + SetError(e.what()); } #else // NAPI_CPP_EXCEPTIONS - self->Execute(); + Execute(); #endif // NAPI_CPP_EXCEPTIONS } -inline void AsyncWorker::OnWorkComplete( - napi_env /*env*/, napi_status status, void* this_pointer) { - AsyncWorker* self = static_cast(this_pointer); +inline void AsyncWorker::OnAsyncWorkComplete(napi_env env, + napi_status status, + void* asyncworker) { + AsyncWorker* self = static_cast(asyncworker); + self->OnWorkComplete(env, status); +} +inline void AsyncWorker::OnWorkComplete(Napi::Env /*env*/, napi_status status) { if (status != napi_cancelled) { - HandleScope scope(self->_env); + HandleScope scope(_env); details::WrapCallback([&] { - if (self->_error.size() == 0) { - self->OnOK(); + if (_error.size() == 0) { + OnOK(); } else { - self->OnError(Error::New(self->_env, self->_error)); + OnError(Error::New(_env, _error)); } return nullptr; }); } - if (!self->_suppress_destruct) { - self->Destroy(); + if (!_suppress_destruct) { + Destroy(); } } @@ -4632,14 +4643,14 @@ inline AsyncProgressWorker::AsyncProgressWorker(const Function& callback, template inline AsyncProgressWorker::AsyncProgressWorker(const Object& receiver, - const Function& callback) + const Function& callback) : AsyncProgressWorker(receiver, callback, "generic") { } template inline AsyncProgressWorker::AsyncProgressWorker(const Object& receiver, - const Function& callback, - const char* resource_name) + const Function& callback, + const char* resource_name) : AsyncProgressWorker(receiver, callback, resource_name, @@ -4665,14 +4676,14 @@ inline AsyncProgressWorker::AsyncProgressWorker(Napi::Env env) template inline AsyncProgressWorker::AsyncProgressWorker(Napi::Env env, - const char* resource_name) + const char* resource_name) : AsyncProgressWorker(env, resource_name, Object::New(env)) { } template inline AsyncProgressWorker::AsyncProgressWorker(Napi::Env env, - const char* resource_name, - const Object& resource) + const char* resource_name, + const Object& resource) : AsyncWorker(env, resource_name, resource), _asyncdata(nullptr), _asyncsize(0) { @@ -4751,7 +4762,6 @@ template inline void AsyncProgressWorker::ExecutionProgress::Send(const T* data, size_t count) const { _worker->SendProgress_(data, count); } - #endif //////////////////////////////////////////////////////////////////////////////// diff --git a/napi.h b/napi.h index 61375b7..49435d5 100644 --- a/napi.h +++ b/napi.h @@ -1986,6 +1986,10 @@ namespace Napi { ObjectReference& Receiver(); FunctionReference& Callback(); + virtual void OnExecute(Napi::Env env); + virtual void OnWorkComplete(Napi::Env env, + napi_status status); + protected: explicit AsyncWorker(const Function& callback); explicit AsyncWorker(const Function& callback, @@ -2019,10 +2023,10 @@ namespace Napi { void SetError(const std::string& error); private: - static void OnExecute(napi_env env, void* this_pointer); - static void OnWorkComplete(napi_env env, - napi_status status, - void* this_pointer); + static inline void OnAsyncWorkExecute(napi_env env, void* asyncworker); + static inline void OnAsyncWorkComplete(napi_env env, + napi_status status, + void* asyncworker); napi_env _env; napi_async_work _work; @@ -2254,33 +2258,32 @@ namespace Napi { }; protected: - explicit AsyncProgressWorker(const Function& callback); - explicit AsyncProgressWorker(const Function& callback, - const char* resource_name); - explicit AsyncProgressWorker(const Function& callback, - const char* resource_name, - const Object& resource); - explicit AsyncProgressWorker(const Object& receiver, - const Function& callback); - explicit AsyncProgressWorker(const Object& receiver, - const Function& callback, - const char* resource_name); - explicit AsyncProgressWorker(const Object& receiver, - const Function& callback, - const char* resource_name, - const Object& resource); + explicit AsyncProgressWorker(const Function& callback); + explicit AsyncProgressWorker(const Function& callback, + const char* resource_name); + explicit AsyncProgressWorker(const Function& callback, + const char* resource_name, + const Object& resource); + explicit AsyncProgressWorker(const Object& receiver, + const Function& callback); + explicit AsyncProgressWorker(const Object& receiver, + const Function& callback, + const char* resource_name); + explicit AsyncProgressWorker(const Object& receiver, + const Function& callback, + const char* resource_name, + const Object& resource); // Optional callback of Napi::ThreadSafeFunction only available after NAPI_VERSION 4. // Refs: https://github.com/nodejs/node/pull/27791 #if NAPI_VERSION > 4 - explicit AsyncProgressWorker(Napi::Env env); - explicit AsyncProgressWorker(Napi::Env env, - const char* resource_name); - explicit AsyncProgressWorker(Napi::Env env, - const char* resource_name, - const Object& resource); + explicit AsyncProgressWorker(Napi::Env env); + explicit AsyncProgressWorker(Napi::Env env, + const char* resource_name); + explicit AsyncProgressWorker(Napi::Env env, + const char* resource_name, + const Object& resource); #endif - virtual void Execute(const ExecutionProgress& progress) = 0; virtual void OnProgress(const T* data, size_t count) = 0;