FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

async_wrap: correctly pass parent to init callback · nodejs/node@3eaa593 · GitHub

/ node Public

Commit 3eaa593

Browse files
trevnorris
authored and
jasnell
committed
async_wrap: correctly pass parent to init callback
Previous logic didn't allow parent to propagate to the init callback properly. The fix now allows the init callback to be called and receive the parent if: - async wrap callbacks are enabled and parent exists - the init callback has been called on the parent and an init callback exists then it will be called regardless of whether async wrap callbacks are disabled. Change the init/pre/post callback checks to see if it has been properly set. This allows removal of the Environment "using_asyncwrap" variable. Pass Isolate to a TryCatch instance. Fixes: #2986 PR-URL: #3216 Reviewed-By: Rod Vagg <rod@vagg.org>
1 parent 28aac7f commit 3eaa593

8 files changed

Lines changed: 131 additions & 40 deletions

File tree

‎src/async-wrap-inl.h‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,32 +24,39 @@ inline AsyncWrap::AsyncWrap(Environment* env,
2424
// Shift provider value over to prevent id collision.
2525
persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider);
2626

27-
// Check user controlled flag to see if the init callback should run.
28-
if (!env->using_asyncwrap())
29-
return;
27+
v8::Local<v8::Function> init_fn = env->async_hooks_init_function();
3028

31-
// If callback hooks have not been enabled, and there is no parent, return.
32-
if (!env->async_wrap_callbacks_enabled() && parent == nullptr)
29+
// No init callback exists, no reason to go on.
30+
if (init_fn.IsEmpty())
3331
return;
3432

35-
// If callback hooks have not been enabled and parent has no queue, return.
36-
if (!env->async_wrap_callbacks_enabled() && !parent->has_async_queue())
33+
// If async wrap callbacks are disabled and no parent was passed that has
34+
// run the init callback then return.
35+
if (!env->async_wrap_callbacks_enabled() &&
36+
(parent == nullptr || !parent->ran_init_callback()))
3737
return;
3838

3939
v8::HandleScope scope(env->isolate());
40-
v8::TryCatch try_catch;
4140

42-
v8::Local<v8::Value> n = v8::Int32::New(env->isolate(), provider);
43-
env->async_hooks_init_function()->Call(object, 1, &n);
41+
v8::Local<v8::Value> argv[] = {
42+
v8::Int32::New(env->isolate(), provider),
43+
Null(env->isolate())
44+
};
45+
46+
if (parent != nullptr)
47+
argv[1] = parent->object();
48+
49+
v8::MaybeLocal<v8::Value> ret =
50+
init_fn->Call(env->context(), object, ARRAY_SIZE(argv), argv);
4451

45-
if (try_catch.HasCaught())
52+
if (ret.IsEmpty())
4653
FatalError("node::AsyncWrap::AsyncWrap", "init hook threw");
4754

48-
bits_ |= 1; // has_async_queue() is true now.
55+
bits_ |= 1; // ran_init_callback() is true now.
4956
}
5057

5158

52-
inline bool AsyncWrap::has_async_queue() const {
59+
inline bool AsyncWrap::ran_init_callback() const {
5360
return static_cast<bool>(bits_ & 1);
5461
}
5562

‎src/async-wrap.cc‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,6 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
123123
env->set_async_hooks_init_function(args[0].As<Function>());
124124
env->set_async_hooks_pre_function(args[1].As<Function>());
125125
env->set_async_hooks_post_function(args[2].As<Function>());
126-
127-
env->set_using_asyncwrap(true);
128126
}
129127

130128

@@ -146,6 +144,10 @@ static void Initialize(Local<Object> target,
146144
NODE_ASYNC_PROVIDER_TYPES(V)
147145
#undef V
148146
target->Set(FIXED_ONE_BYTE_STRING(isolate, "Providers"), async_providers);
147+
148+
env->set_async_hooks_init_function(Local<Function>());
149+
env->set_async_hooks_pre_function(Local<Function>());
150+
env->set_async_hooks_post_function(Local<Function>());
149151
}
150152

151153

@@ -164,6 +166,8 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
164166
Local<Value>* argv) {
165167
CHECK(env()->context() == env()->isolate()->GetCurrentContext());
166168

169+
Local<Function> pre_fn = env()->async_hooks_pre_function();
170+
Local<Function> post_fn = env()->async_hooks_post_function();
167171
Local<Object> context = object();
168172
Local<Object> process = env()->process_object();
169173
Local<Object> domain;
@@ -179,7 +183,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
179183
}
180184
}
181185

182-
TryCatch try_catch;
186+
TryCatch try_catch(env()->isolate());
183187
try_catch.SetVerbose(true);
184188

185189
if (has_domain) {
@@ -191,9 +195,9 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
191195
}
192196
}
193197

194-
if (has_async_queue()) {
198+
if (ran_init_callback() && !pre_fn.IsEmpty()) {
195199
try_catch.SetVerbose(false);
196-
env()->async_hooks_pre_function()->Call(context, 0, nullptr);
200+
pre_fn->Call(context, 0, nullptr);
197201
if (try_catch.HasCaught())
198202
FatalError("node::AsyncWrap::MakeCallback", "pre hook threw");
199203
try_catch.SetVerbose(true);
@@ -205,9 +209,9 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
205209
return Undefined(env()->isolate());
206210
}
207211

208-
if (has_async_queue()) {
212+
if (ran_init_callback() && !post_fn.IsEmpty()) {
209213
try_catch.SetVerbose(false);
210-
env()->async_hooks_post_function()->Call(context, 0, nullptr);
214+
post_fn->Call(context, 0, nullptr);
211215
if (try_catch.HasCaught())
212216
FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
213217
try_catch.SetVerbose(true);

‎src/async-wrap.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ class AsyncWrap : public BaseObject {
7070

7171
private:
7272
inline AsyncWrap();
73-
inline bool has_async_queue() const;
73+
inline bool ran_init_callback() const;
7474

7575
// When the async hooks init JS function is called from the constructor it is
7676
// expected the context object will receive a _asyncQueue object property

‎src/env-inl.h‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,6 @@ inline Environment::Environment(v8::Local<v8::Context> context,
208208
isolate_data_(IsolateData::GetOrCreate(context->GetIsolate(), loop)),
209209
timer_base_(uv_now(loop)),
210210
using_domains_(false),
211-
using_asyncwrap_(false),
212211
printed_error_(false),
213212
trace_sync_io_(false),
214213
debugger_agent_(this),
@@ -348,14 +347,6 @@ inline void Environment::set_using_domains(bool value) {
348347
using_domains_ = value;
349348
}
350349

351-
inline bool Environment::using_asyncwrap() const {
352-
return using_asyncwrap_;
353-
}
354-
355-
inline void Environment::set_using_asyncwrap(bool value) {
356-
using_asyncwrap_ = value;
357-
}
358-
359350
inline bool Environment::printed_error() const {
360351
return printed_error_;
361352
}

‎src/env.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -435,9 +435,6 @@ class Environment {
435435
inline bool using_domains() const;
436436
inline void set_using_domains(bool value);
437437

438-
inline bool using_asyncwrap() const;
439-
inline void set_using_asyncwrap(bool value);
440-
441438
inline bool printed_error() const;
442439
inline void set_printed_error(bool value);
443440

@@ -537,7 +534,6 @@ class Environment {
537534
ares_channel cares_channel_;
538535
ares_task_list cares_task_list_;
539536
bool using_domains_;
540-
bool using_asyncwrap_;
541537
bool printed_error_;
542538
bool trace_sync_io_;
543539
debugger::Agent debugger_agent_;

‎src/node.cc‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1040,15 +1040,19 @@ Local<Value> MakeCallback(Environment* env,
10401040
// If you hit this assertion, you forgot to enter the v8::Context first.
10411041
CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
10421042

1043+
Local<Function> pre_fn = env->async_hooks_pre_function();
1044+
Local<Function> post_fn = env->async_hooks_post_function();
10431045
Local<Object> object, domain;
1044-
bool has_async_queue = false;
1046+
bool ran_init_callback = false;
10451047
bool has_domain = false;
10461048

1049+
// TODO(trevnorris): Adding "_asyncQueue" to the "this" in the init callback
1050+
// is a horrible way to detect usage. Rethink how detection should happen.
10471051
if (recv->IsObject()) {
10481052
object = recv.As<Object>();
10491053
Local<Value> async_queue_v = object->Get(env->async_queue_string());
10501054
if (async_queue_v->IsObject())
1051-
has_async_queue = true;
1055+
ran_init_callback = true;
10521056
}
10531057

10541058
if (env->using_domains()) {
@@ -1074,19 +1078,19 @@ Local<Value> MakeCallback(Environment* env,
10741078
}
10751079
}
10761080

1077-
if (has_async_queue) {
1081+
if (ran_init_callback && !pre_fn.IsEmpty()) {
10781082
try_catch.SetVerbose(false);
1079-
env->async_hooks_pre_function()->Call(object, 0, nullptr);
1083+
pre_fn->Call(object, 0, nullptr);
10801084
if (try_catch.HasCaught())
10811085
FatalError("node::MakeCallback", "pre hook threw");
10821086
try_catch.SetVerbose(true);
10831087
}
10841088

10851089
Local<Value> ret = callback->Call(recv, argc, argv);
10861090

1087-
if (has_async_queue) {
1091+
if (ran_init_callback && !post_fn.IsEmpty()) {
10881092
try_catch.SetVerbose(false);
1089-
env->async_hooks_post_function()->Call(object, 0, nullptr);
1093+
post_fn->Call(object, 0, nullptr);
10901094
if (try_catch.HasCaught())
10911095
FatalError("node::MakeCallback", "post hook threw");
10921096
try_catch.SetVerbose(true);

‎test/parallel/test-async-wrap-disabled-propagate-parent.js‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const net = require('net');
6+
const async_wrap = process.binding('async_wrap');
7+
const providers = Object.keys(async_wrap.Providers);
8+
9+
let cntr = 0;
10+
let server;
11+
let client;
12+
13+
function init(type, parent) {
14+
if (parent) {
15+
cntr++;
16+
// Cannot assert in init callback or will abort.
17+
process.nextTick(() => {
18+
assert.equal(providers[type], 'TCPWRAP');
19+
assert.equal(parent, server._handle, 'server doesn\'t match parent');
20+
assert.equal(this, client._handle, 'client doesn\'t match context');
21+
});
22+
}
23+
}
24+
25+
function noop() { }
26+
27+
async_wrap.setupHooks(init, noop, noop);
28+
async_wrap.enable();
29+
30+
server = net.createServer(function(c) {
31+
client = c;
32+
// Allow init callback to run before closing.
33+
setImmediate(() => {
34+
c.end();
35+
this.close();
36+
});
37+
}).listen(common.PORT, function() {
38+
net.connect(common.PORT, noop);
39+
});
40+
41+
async_wrap.disable();
42+
43+
process.on('exit', function() {
44+
// init should have only been called once with a parent.
45+
assert.equal(cntr, 1);
46+
});

‎test/parallel/test-async-wrap-propagate-parent.js‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const net = require('net');
6+
const async_wrap = process.binding('async_wrap');
7+
8+
let cntr = 0;
9+
let server;
10+
let client;
11+
12+
function init(type, parent) {
13+
if (parent) {
14+
cntr++;
15+
// Cannot assert in init callback or will abort.
16+
process.nextTick(() => {
17+
assert.equal(parent, server._handle, 'server doesn\'t match parent');
18+
assert.equal(this, client._handle, 'client doesn\'t match context');
19+
});
20+
}
21+
}
22+
23+
function noop() { }
24+
25+
async_wrap.setupHooks(init, noop, noop);
26+
async_wrap.enable();
27+
28+
server = net.createServer(function(c) {
29+
client = c;
30+
// Allow init callback to run before closing.
31+
setImmediate(() => {
32+
c.end();
33+
this.close();
34+
});
35+
}).listen(common.PORT, function() {
36+
net.connect(common.PORT, noop);
37+
});
38+
39+
40+
process.on('exit', function() {
41+
// init should have only been called once with a parent.
42+
assert.equal(cntr, 1);
43+
});

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL