Test and fix AsyncWorker #30
Conversation
|
@digitalinfinity please review, this is somewhat based on your feedback. |
napi.h
Outdated
| @@ -850,18 +846,16 @@ namespace Napi { | |||
| void Queue(); | |||
| void Cancel(); | |||
|
|
|||
| virtual void Execute() = 0; | |||
| virtual void WorkComplete(); | |||
|
|
|||
| ObjectReference& Persistent(); | |||
|
|
|||
| protected: | |||
| explicit AsyncWorker(const Function& callback); | |||
There was a problem hiding this comment.
Were you still planning on having an overload that allowed the user to pass in the this pointer for use with the callbacks?
There was a problem hiding this comment.
I had been thinking that another object reference would be wasteful, but now I realize it could make sense to use the receiver object as the _persistent object reference, if one was supplied, instead of creating a new object. I'll update this PR.
napi.h
Outdated
| virtual void OnOK(); | ||
| virtual void OnError(Error e); | ||
|
|
||
| void SetError(Error error); | ||
| void SetError(std::string error); |
There was a problem hiding this comment.
This should be const std::string&
napi-inl.h
Outdated
| catch (const Error& e) { | ||
| self->SetError(e); | ||
| } | ||
| self->Execute(); |
There was a problem hiding this comment.
We could still store error.what() for any std::exception we run into … anything else is going to hard crash the process anyway, afaict?
|
I was working on updating this PR, but after adding more tests I ran into a bug #31. I'm going to fix that first, then come back to this. |
|
I rebased on top of the error fix, and updated to address review feedback so far. So when reviewing here, please only look at the second commit. I encountered an interesting problem: how to deal with unhandled JS exceptions during an async callback. See the TODO comment in |
|
Related to the TODO mentioned above, I'm working on a change in the C APIs that will call |
|
Related node PR: nodejs/node#12838 |
- Remove MakeCallback overload that defaulted to undefined receiver, because node::MakeCallback requires an object. - Allow the AsyncWorker constructor to optionally specify a receiver object, that is persisted and used as of an async callback. - Persist async errors as strings, because an Error object cannot be created outside of a JS context. - Remove overridable AsyncWorker::WorkComplete() because it wasn't useful and caused confusion. OnOK() and/or OnError() should be (optionally) overridden instead. - Add tests to validate basic success and error scenarios for AsyncWorker. - Also add necessary cast to Object when calling Unwrap.
After nodejs/node#12838 is merged, the exception will be properly reported.
MakeCallback()overloads that defaulted to undefined receiver, becausenode::MakeCallback()requires an object.Errorobject cannot be created outside of a JS contextAsyncWorker::WorkComplete()because it wasn't useful and caused confusion.OnOK()and/orOnError()should be (optionally) overridden instead.AsyncWorker