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.

Sign up for GitHub

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

Jump to bottom

Test and fix AsyncWorker #30

Merged
merged 2 commits into from
May 4, 2017
Merged

Test and fix AsyncWorker #30

merged 2 commits into from
May 4, 2017

Conversation

jasongin
Copy link
Member

  • Remove MakeCallback() overloads that defaulted to undefined receiver, because node::MakeCallback() requires an object.
  • Use the async worker persistent object as the receiver of a worker 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

@jasongin
Copy link
Member Author

jasongin commented Apr 27, 2017 •
edited
Loading

@digitalinfinity please review, this is somewhat based on your feedback.

digitalinfinity
digitalinfinity reviewed Apr 27, 2017
View reviewed changes
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Were you still planning on having an overload that allowed the user to pass in the this pointer for use with the callbacks?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor 

T* instance = Unwrap(callbackInfo.This());

While you're touching this file, can you also fix this to convert this to an object since that's what Unwrap expects?

Refers to: napi-inl.h:2001 in 9a0062a. [](commit_id = 9a0062a, deletion_comment = False)

mhdawson
mhdawson approved these changes Apr 28, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

addaleax
addaleax reviewed Apr 28, 2017
View reviewed changes
napi.h Outdated
virtual void OnOK();
virtual void OnError(Error e);

void SetError(Error error);
void SetError(std::string error);
Copy link
Member

Choose a reason for hiding this comment

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

This should be const std::string&

napi-inl.h Outdated
catch (const Error& e) {
self->SetError(e);
}
self->Execute();
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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.

@jasongin
Copy link
Member Author

jasongin commented May 1, 2017 •
edited
Loading

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 AsyncWorker::OnWorkComplete(). Apparently this is something that Nan (or node::MakeCallback()) doesn't really address, but ideally this wrapper should do something better to provide consistent error-handling behavior.

@jasongin
Copy link
Member Author

jasongin commented May 3, 2017

Related to the TODO mentioned above, I'm working on a change in the C APIs that will call node::FatalException() (which is a public API, unlike node::FatalError) if a JavaScript exception escapes from a napi_async_complete_callback. Then AsyncWorker::OnWorkComplete() should just rethrow any Napi::Error as a JavaScript exception (just like other function callbacks already do via the NAPI_RETHROW_JS_ERROR macro).

boingoing
boingoing approved these changes May 4, 2017
View reviewed changes
Copy link

@boingoing boingoing left a comment

Choose a reason for hiding this comment

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

LGTM

@jasongin
Copy link
Member Author

jasongin commented May 4, 2017

Related node PR: nodejs/node#12838

@jasongin
Test and fix AsyncWorker
4236269 
 - 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.
@jasongin
Rethrow async cb error as JS exception
cb46bc5 
After nodejs/node#12838 is merged, the exception will be properly reported.
@jasongin jasongin merged commit 3066c06 into nodejs:master May 4, 2017
@jasongin jasongin deleted the asyncworker branch May 4, 2017 23:00
@JckXia JckXia mentioned this pull request Mar 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@mhdawson mhdawson mhdawson approved these changes

@boingoing boingoing boingoing approved these changes

@digitalinfinity digitalinfinity digitalinfinity approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jasongin @digitalinfinity @addaleax @boingoing @mhdawson