Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: make OnWorkComplete and OnExecute override-able #589

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions doc/async_worker.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
This method is invoked after the work has completed on JavaScript thread.
This method is invoked after the work has completed on the 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`
legendecas marked this conversation as resolved.
Show resolved Hide resolved
and handles exceptions if cpp exceptions were enabled.

gabrielschulhof marked this conversation as resolved.
Show resolved Hide resolved
The `OnExecute` method receives an `napi_env` argument. However, do NOT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The `OnExecute` method receives an `napi_env` argument. However, do NOT
The `OnExecute` method receives an `napi_env` argument. However, it must NOT be used

use it within this method, as it does not run on the JavaScript thread and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use it within this method, as it does not run on the JavaScript thread and
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
Expand Down
66 changes: 38 additions & 28 deletions napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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);
}

Expand Down Expand Up @@ -4211,40 +4211,51 @@ inline void AsyncWorker::SetError(const std::string& error) {
inline std::vector<napi_value> 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*>(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<AsyncWorker*>(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<AsyncWorker*>(this_pointer);
inline void AsyncWorker::OnAsyncWorkComplete(napi_env env,
napi_status status,
void* asyncworker) {
AsyncWorker* self = static_cast<AsyncWorker*>(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();
}
}

Expand Down Expand Up @@ -4609,14 +4620,14 @@ inline AsyncProgressWorker<T>::AsyncProgressWorker(const Function& callback,

template<class T>
inline AsyncProgressWorker<T>::AsyncProgressWorker(const Object& receiver,
const Function& callback)
const Function& callback)
: AsyncProgressWorker(receiver, callback, "generic") {
}

template<class T>
inline AsyncProgressWorker<T>::AsyncProgressWorker(const Object& receiver,
const Function& callback,
const char* resource_name)
const Function& callback,
const char* resource_name)
: AsyncProgressWorker(receiver,
callback,
resource_name,
Expand All @@ -4642,14 +4653,14 @@ inline AsyncProgressWorker<T>::AsyncProgressWorker(Napi::Env env)

template<class T>
inline AsyncProgressWorker<T>::AsyncProgressWorker(Napi::Env env,
const char* resource_name)
const char* resource_name)
: AsyncProgressWorker(env, resource_name, Object::New(env)) {
}

template<class T>
inline AsyncProgressWorker<T>::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) {
Expand Down Expand Up @@ -4728,7 +4739,6 @@ template<class T>
inline void AsyncProgressWorker<T>::ExecutionProgress::Send(const T* data, size_t count) const {
_worker->SendProgress_(data, count);
}

#endif

////////////////////////////////////////////////////////////////////////////////
Expand Down
55 changes: 29 additions & 26 deletions napi.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down