From 0de964cdb3634bb6fb5664c9828ea25ea11b1da3 Mon Sep 17 00:00:00 2001 From: legendecas Date: Wed, 6 Nov 2019 20:01:48 +0800 Subject: [PATCH 1/3] 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`. --- doc/async_worker.md | 29 ++++++++++++++++++++++ napi-inl.h | 60 ++++++++++++++++++++++++++------------------- napi.h | 55 +++++++++++++++++++++-------------------- 3 files changed, 93 insertions(+), 51 deletions(-) diff --git a/doc/async_worker.md b/doc/async_worker.md index 88668af07..21c9e3a1f 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, 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. + +```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 7bef465fc..16f72d0ae 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -4097,8 +4097,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); } @@ -4123,8 +4123,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); } @@ -4211,40 +4211,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 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::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); +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(); } } @@ -4609,14 +4620,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, @@ -4642,14 +4653,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) { @@ -4728,7 +4739,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 c44b3ba92..f483e39ce 100644 --- a/napi.h +++ b/napi.h @@ -1991,6 +1991,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, @@ -2024,10 +2028,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; @@ -2259,33 +2263,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; From 02c69bcae0ab99ac3cd38a086fb7510e8a858eb1 Mon Sep 17 00:00:00 2001 From: legendecas Date: Wed, 1 Jan 2020 21:17:56 +0800 Subject: [PATCH 2/3] fixup! src: make OnWorkComplete and OnExecute override-able --- doc/async_worker.md | 6 +++--- napi-inl.h | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/doc/async_worker.md b/doc/async_worker.md index 21c9e3a1f..f54d2bce4 100644 --- a/doc/async_worker.md +++ b/doc/async_worker.md @@ -157,9 +157,9 @@ The default implementation of this method just calls the `Napi::AsyncWorker::Exe and handles exceptions if cpp exceptions were enabled. 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. +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. ```cpp virtual void OnExecute(Napi::Env env); diff --git a/napi-inl.h b/napi-inl.h index 16f72d0ae..37e9b673b 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -4212,17 +4212,17 @@ 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 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. +// 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. +// 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 { From b117b43d2f05dd7c219c0e9a73566ab8782c4b10 Mon Sep 17 00:00:00 2001 From: legendecas Date: Sun, 5 Jan 2020 13:19:36 +0800 Subject: [PATCH 3/3] fixup! src: make OnWorkComplete and OnExecute override-able --- doc/async_worker.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/async_worker.md b/doc/async_worker.md index f54d2bce4..2573cd2e6 100644 --- a/doc/async_worker.md +++ b/doc/async_worker.md @@ -156,10 +156,10 @@ 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, 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. +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);