Test and fix AsyncWorker#30
Uh oh!
There was an error while loading. Please reload this page.
Conversation
@digitalinfinity please review, this is somewhat based on your feedback. |
| ObjectReference& Persistent(); | ||
| protected: | ||
| explicitAsyncWorker(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.
digitalinfinity commented Apr 27, 2017
napi.h Outdated
| virtualvoidOnError(Error e); | ||
| voidSetError(Error error); | ||
| voidSetError(std::string error); |
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?
jasongin commented Apr 29, 2017
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 |
jasongin commented May 3, 2017
Related to the TODO mentioned above, I'm working on a change in the C APIs that will call |
jasongin commented May 4, 2017
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.
5 participants
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