Skip to content

async_hooks,async_wrap: fixups and fewer aborts #14722

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

Closed
trevnorris wants to merge 4 commits into from
Closed

async_hooks,async_wrap: fixups and fewer aborts #14722

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f5ef678
async_wrap: return undefined if domain is disposed
trevnorris Aug 9, 2017
0f863d0
async_hooks: improve comments and function names
trevnorris Aug 2, 2017
f2c1638
async_wrap: unroll unnecessarily DRY code
trevnorris Aug 3, 2017
14c3a5e
async_hooks: don't abort unnecessarily
trevnorris Aug 3, 2017
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
Next Next commit
async_wrap: return undefined if domain is disposed 
v8::MaybeLocal::ToLocalChecked() will abort on an empty handle.
AsyncWrap::MakeCallback returns an empty handle if the domain is
disposed, but this should not cause the process to abort. So instead
return v8::Undefined() and allow the error to be handled normally.

PR-URL: #14722
  • Loading branch information
@trevnorris
trevnorris committed Aug 11, 2017
commit f5ef678bd896b07e4009a785c85e15f5fde7d2d0
5 changes: 3 additions & 2 deletions src/async-wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ using v8::String;
using v8::Symbol;
using v8::TryCatch;
using v8::Uint32Array;
using v8::Undefined;
using v8::Value;

using AsyncHooks = node::Environment::AsyncHooks;
Expand Down Expand Up @@ -687,7 +688,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
get_trigger_id());

if (!PreCallbackExecution(this, true)) {
return MaybeLocal<Value>();
return Undefined(env()->isolate());
}

// Finally... Get to running the user's callback.
Expand All @@ -698,7 +699,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}

if (!PostCallbackExecution(this, true)) {
return Local<Value>();
return Undefined(env()->isolate());
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 10, 2017

Choose a reason for hiding this comment

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

Please also take a look at #14697 when you get the chance.

}

exec_scope.Dispose();
Expand Down
25 changes: 25 additions & 0 deletions test/parallel/test-domain-abort-when-disposed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain');

// These were picked arbitrarily and are only used to trigger arync_hooks.
const JSStream = process.binding('js_stream').JSStream;
const Socket = require('net').Socket;

const handle = new JSStream();
handle.domain = domain.create();
handle.domain.dispose();

handle.close = () => {};
handle.isAlive = () => { throw new Error(); };

const s = new Socket({ handle });

// When AsyncWrap::MakeCallback() returned an empty handle the
// MaybeLocal::ToLocalChecked() call caused the process to abort. By returning
// v8::Undefined() it allows the error to propagate to the 'error' event.
s.on('error', common.mustCall((e) => {
Copy link
Copy Markdown
Contributor

@refack refack Aug 10, 2017

Choose a reason for hiding this comment

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

What triggers this? the dispose on next tick?

Copy link
Copy Markdown
Contributor Author

@trevnorris trevnorris Aug 11, 2017

Choose a reason for hiding this comment

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

The abort or the 'error' event?

The abort is because we're currently returning Local<Value>() from AsyncWrap::MakeCallback(); which now returns a MaybeLocal<Value>. So it will abort on MaybeLocal::ToLocalChecked() if the domain is disposed. Instead of propagating the error correctly.

Copy link
Copy Markdown
Contributor

@refack refack Aug 11, 2017

Choose a reason for hiding this comment

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

the error event? since there is not interaction with s, I'm assuming that it's some cleanup code that triggers the error event.

Copy link
Copy Markdown
Contributor Author

@trevnorris trevnorris Aug 11, 2017

Choose a reason for hiding this comment

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

@refack implementation details of how the JSStream instance dictates how the Socket connects. Because the JSStream instance isn't connected to anything the socket reports EINVAL.

assert.strictEqual(e.code, 'EINVAL');
}));