Skip to content

Commit 3b8da4c

Browse files
committed
async_hooks: use scope for defaultTriggerAsyncId
Previously the getter would mutate the kDefaultTriggerAsncId value. This refactor changes the setter to bind the current kDefaultTriggerAsncId to a scope, such that the getter doesn't have to mutate its own value. PR-URL: #17273 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 0784b04 commit 3b8da4c

File tree

12 files changed

+142
-121
lines changed

12 files changed

+142
-121
lines changed

lib/dgram.js

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ const dns = require('dns');
2828
const util = require('util');
2929
const { isUint8Array } = require('internal/util/types');
3030
const EventEmitter = require('events');
31-
const { setDefaultTriggerAsyncId } = require('internal/async_hooks');
31+
const { defaultTriggerAsyncIdScope } = require('internal/async_hooks');
3232
const { UV_UDP_REUSEADDR } = process.binding('constants').os;
3333
const { async_id_symbol } = process.binding('async_wrap');
3434
const { nextTick } = require('internal/process/next_tick');
@@ -450,21 +450,24 @@ Socket.prototype.send = function(buffer,
450450
}
451451

452452
const afterDns = (ex, ip) => {
453-
doSend(ex, this, ip, list, address, port, callback);
453+
defaultTriggerAsyncIdScope(
454+
this[async_id_symbol],
455+
[ex, this, ip, list, address, port, callback],
456+
doSend
457+
);
454458
};
455459

456460
this._handle.lookup(address, afterDns);
457461
};
458462

459-
460463
function doSend(ex, self, ip, list, address, port, callback) {
461464
if (ex) {
462465
if (typeof callback === 'function') {
463-
callback(ex);
466+
process.nextTick(callback, ex);
464467
return;
465468
}
466469

467-
self.emit('error', ex);
470+
process.nextTick(() => self.emit('error', ex));
468471
return;
469472
} else if (!self._handle) {
470473
return;
@@ -478,20 +481,18 @@ function doSend(ex, self, ip, list, address, port, callback) {
478481
req.callback = callback;
479482
req.oncomplete = afterSend;
480483
}
481-
// node::SendWrap isn't instantiated and attached to the JS instance of
482-
// SendWrap above until send() is called. So don't set the init trigger id
483-
// until now.
484-
setDefaultTriggerAsyncId(self[async_id_symbol]);
484+
485485
var err = self._handle.send(req,
486486
list,
487487
list.length,
488488
port,
489489
ip,
490490
!!callback);
491+
491492
if (err && callback) {
492493
// don't emit as error, dgram_legacy.js compatibility
493494
const ex = exceptionWithHostPort(err, 'send', address, port);
494-
nextTick(self[async_id_symbol], callback, ex);
495+
process.nextTick(callback, ex);
495496
}
496497
}
497498

lib/internal/async_hooks.js

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -250,20 +250,27 @@ function newUid() {
250250
// constructor is complete.
251251
function getDefaultTriggerAsyncId() {
252252
var defaultTriggerAsyncId = async_id_fields[kDefaultTriggerAsyncId];
253-
// Reset value after it's been called so the next constructor doesn't
254-
// inherit it by accident.
255-
async_id_fields[kDefaultTriggerAsyncId] = -1;
256253
// If defaultTriggerAsyncId isn't set, use the executionAsyncId
257254
if (defaultTriggerAsyncId < 0)
258255
defaultTriggerAsyncId = async_id_fields[kExecutionAsyncId];
259256
return defaultTriggerAsyncId;
260257
}
261258

262259

263-
function setDefaultTriggerAsyncId(triggerAsyncId) {
260+
function defaultTriggerAsyncIdScope(triggerAsyncId, opaque, block) {
264261
// CHECK(Number.isSafeInteger(triggerAsyncId))
265262
// CHECK(triggerAsyncId > 0)
263+
const oldDefaultTriggerAsyncId = async_id_fields[kDefaultTriggerAsyncId];
266264
async_id_fields[kDefaultTriggerAsyncId] = triggerAsyncId;
265+
266+
var ret;
267+
try {
268+
ret = Reflect.apply(block, null, opaque);
269+
} finally {
270+
async_id_fields[kDefaultTriggerAsyncId] = oldDefaultTriggerAsyncId;
271+
}
272+
273+
return ret;
267274
}
268275

269276

@@ -285,10 +292,6 @@ function emitInitScript(asyncId, type, triggerAsyncId, resource) {
285292
// manually means that the embedder must have used getDefaultTriggerAsyncId().
286293
if (triggerAsyncId === null) {
287294
triggerAsyncId = getDefaultTriggerAsyncId();
288-
} else {
289-
// If a triggerAsyncId was passed, any kDefaultTriggerAsyncId still must be
290-
// null'd.
291-
async_id_fields[kDefaultTriggerAsyncId] = -1;
292295
}
293296

294297
emitInitNative(asyncId, type, triggerAsyncId, resource);
@@ -341,7 +344,7 @@ module.exports = {
341344
// Internal Embedder API
342345
newUid,
343346
getDefaultTriggerAsyncId,
344-
setDefaultTriggerAsyncId,
347+
defaultTriggerAsyncIdScope,
345348
emitInit: emitInitScript,
346349
emitBefore: emitBeforeScript,
347350
emitAfter: emitAfterScript,

lib/net.js

Lines changed: 46 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ const { TCPConnectWrap } = process.binding('tcp_wrap');
4343
const { PipeConnectWrap } = process.binding('pipe_wrap');
4444
const { ShutdownWrap, WriteWrap } = process.binding('stream_wrap');
4545
const { async_id_symbol } = process.binding('async_wrap');
46-
const { newUid, setDefaultTriggerAsyncId } = require('internal/async_hooks');
46+
const { newUid, defaultTriggerAsyncIdScope } = require('internal/async_hooks');
4747
const { nextTick } = require('internal/process/next_tick');
4848
const errors = require('internal/errors');
4949
const dns = require('dns');
@@ -277,6 +277,14 @@ Socket.prototype._unrefTimer = function _unrefTimer() {
277277
timers._unrefActive(s);
278278
};
279279

280+
281+
function shutdownSocket(self, callback) {
282+
var req = new ShutdownWrap();
283+
req.oncomplete = callback;
284+
req.handle = self._handle;
285+
return self._handle.shutdown(req);
286+
}
287+
280288
// the user has called .end(), and all the bytes have been
281289
// sent out to the other side.
282290
function onSocketFinish() {
@@ -298,14 +306,9 @@ function onSocketFinish() {
298306
if (!this._handle || !this._handle.shutdown)
299307
return this.destroy();
300308

301-
var req = new ShutdownWrap();
302-
req.oncomplete = afterShutdown;
303-
req.handle = this._handle;
304-
// node::ShutdownWrap isn't instantiated and attached to the JS instance of
305-
// ShutdownWrap above until shutdown() is called. So don't set the init
306-
// trigger id until now.
307-
setDefaultTriggerAsyncId(this[async_id_symbol]);
308-
var err = this._handle.shutdown(req);
309+
var err = defaultTriggerAsyncIdScope(
310+
this[async_id_symbol], [this, afterShutdown], shutdownSocket
311+
);
309312

310313
if (err)
311314
return this.destroy(errnoException(err, 'shutdown'));
@@ -945,23 +948,15 @@ function internalConnect(
945948
req.localAddress = localAddress;
946949
req.localPort = localPort;
947950

948-
// node::TCPConnectWrap isn't instantiated and attached to the JS instance
949-
// of TCPConnectWrap above until connect() is called. So don't set the init
950-
// trigger id until now.
951-
setDefaultTriggerAsyncId(self[async_id_symbol]);
952951
if (addressType === 4)
953952
err = self._handle.connect(req, address, port);
954953
else
955954
err = self._handle.connect6(req, address, port);
956-
957955
} else {
958956
const req = new PipeConnectWrap();
959957
req.address = address;
960958
req.oncomplete = afterConnect;
961-
// node::PipeConnectWrap isn't instantiated and attached to the JS instance
962-
// of PipeConnectWrap above until connect() is called. So don't set the
963-
// init trigger id until now.
964-
setDefaultTriggerAsyncId(self[async_id_symbol]);
959+
965960
err = self._handle.connect(req, address, afterConnect);
966961
}
967962

@@ -1030,7 +1025,9 @@ Socket.prototype.connect = function(...args) {
10301025
'string',
10311026
path);
10321027
}
1033-
internalConnect(this, path);
1028+
defaultTriggerAsyncIdScope(
1029+
this[async_id_symbol], [this, path], internalConnect
1030+
);
10341031
} else {
10351032
lookupAndConnect(this, options);
10361033
}
@@ -1073,7 +1070,11 @@ function lookupAndConnect(self, options) {
10731070
if (addressType) {
10741071
nextTick(self[async_id_symbol], function() {
10751072
if (self.connecting)
1076-
internalConnect(self, host, port, addressType, localAddress, localPort);
1073+
defaultTriggerAsyncIdScope(
1074+
self[async_id_symbol],
1075+
[self, host, port, addressType, localAddress, localPort],
1076+
internalConnect
1077+
);
10771078
});
10781079
return;
10791080
}
@@ -1097,33 +1098,33 @@ function lookupAndConnect(self, options) {
10971098
debug('connect: dns options', dnsopts);
10981099
self._host = host;
10991100
var lookup = options.lookup || dns.lookup;
1100-
setDefaultTriggerAsyncId(self[async_id_symbol]);
1101-
lookup(host, dnsopts, function emitLookup(err, ip, addressType) {
1102-
self.emit('lookup', err, ip, addressType, host);
1101+
defaultTriggerAsyncIdScope(self[async_id_symbol], [], function() {
1102+
lookup(host, dnsopts, function emitLookup(err, ip, addressType) {
1103+
self.emit('lookup', err, ip, addressType, host);
11031104

1104-
// It's possible we were destroyed while looking this up.
1105-
// XXX it would be great if we could cancel the promise returned by
1106-
// the look up.
1107-
if (!self.connecting) return;
1105+
// It's possible we were destroyed while looking this up.
1106+
// XXX it would be great if we could cancel the promise returned by
1107+
// the look up.
1108+
if (!self.connecting) return;
11081109

1109-
if (err) {
1110-
// net.createConnection() creates a net.Socket object and
1111-
// immediately calls net.Socket.connect() on it (that's us).
1112-
// There are no event listeners registered yet so defer the
1113-
// error event to the next tick.
1114-
err.host = options.host;
1115-
err.port = options.port;
1116-
err.message = err.message + ' ' + options.host + ':' + options.port;
1117-
process.nextTick(connectErrorNT, self, err);
1118-
} else {
1119-
self._unrefTimer();
1120-
internalConnect(self,
1121-
ip,
1122-
port,
1123-
addressType,
1124-
localAddress,
1125-
localPort);
1126-
}
1110+
if (err) {
1111+
// net.createConnection() creates a net.Socket object and
1112+
// immediately calls net.Socket.connect() on it (that's us).
1113+
// There are no event listeners registered yet so defer the
1114+
// error event to the next tick.
1115+
err.host = options.host;
1116+
err.port = options.port;
1117+
err.message = err.message + ' ' + options.host + ':' + options.port;
1118+
process.nextTick(connectErrorNT, self, err);
1119+
} else {
1120+
self._unrefTimer();
1121+
defaultTriggerAsyncIdScope(
1122+
self[async_id_symbol],
1123+
[self, ip, port, addressType, localAddress, localPort],
1124+
internalConnect
1125+
);
1126+
}
1127+
});
11271128
});
11281129
}
11291130

src/async_wrap.cc

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -308,12 +308,13 @@ static void PromiseHook(PromiseHookType type, Local<Promise> promise,
308308
if (parent_wrap == nullptr) {
309309
parent_wrap = PromiseWrap::New(env, parent_promise, nullptr, true);
310310
}
311-
// get id from parentWrap
312-
double trigger_async_id = parent_wrap->get_async_id();
313-
env->set_default_trigger_async_id(trigger_async_id);
314-
}
315311

316-
wrap = PromiseWrap::New(env, promise, parent_wrap, silent);
312+
AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(
313+
env, parent_wrap->get_async_id());
314+
wrap = PromiseWrap::New(env, promise, parent_wrap, silent);
315+
} else {
316+
wrap = PromiseWrap::New(env, promise, nullptr, silent);
317+
}
317318
}
318319

319320
CHECK_NE(wrap, nullptr);

src/connection_wrap.cc

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@ void ConnectionWrap<WrapType, UVType>::OnConnection(uv_stream_t* handle,
4949
};
5050

5151
if (status == 0) {
52-
env->set_default_trigger_async_id(wrap_data->get_async_id());
5352
// Instantiate the client javascript object and handle.
5453
Local<Object> client_obj = WrapType::Instantiate(env,
5554
wrap_data,

src/env-inl.h

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -176,23 +176,27 @@ inline void Environment::AsyncHooks::clear_async_id_stack() {
176176
async_id_fields_[kTriggerAsyncId] = 0;
177177
}
178178

179-
inline Environment::AsyncHooks::InitScope::InitScope(
180-
Environment* env, double init_trigger_async_id)
181-
: env_(env),
182-
async_id_fields_ref_(env->async_hooks()->async_id_fields()) {
183-
if (env_->async_hooks()->fields()[AsyncHooks::kCheck] > 0) {
184-
CHECK_GE(init_trigger_async_id, -1);
179+
inline Environment::AsyncHooks::DefaultTriggerAsyncIdScope
180+
::DefaultTriggerAsyncIdScope(Environment* env,
181+
double default_trigger_async_id)
182+
: async_id_fields_ref_(env->async_hooks()->async_id_fields()) {
183+
if (env->async_hooks()->fields()[AsyncHooks::kCheck] > 0) {
184+
CHECK_GE(default_trigger_async_id, 0);
185185
}
186-
env->async_hooks()->push_async_ids(
187-
async_id_fields_ref_[AsyncHooks::kExecutionAsyncId],
188-
init_trigger_async_id);
186+
187+
old_default_trigger_async_id_ =
188+
async_id_fields_ref_[AsyncHooks::kDefaultTriggerAsyncId];
189+
async_id_fields_ref_[AsyncHooks::kDefaultTriggerAsyncId] =
190+
default_trigger_async_id;
189191
}
190192

191-
inline Environment::AsyncHooks::InitScope::~InitScope() {
192-
env_->async_hooks()->pop_async_id(
193-
async_id_fields_ref_[AsyncHooks::kExecutionAsyncId]);
193+
inline Environment::AsyncHooks::DefaultTriggerAsyncIdScope
194+
::~DefaultTriggerAsyncIdScope() {
195+
async_id_fields_ref_[AsyncHooks::kDefaultTriggerAsyncId] =
196+
old_default_trigger_async_id_;
194197
}
195198

199+
196200
inline Environment::AsyncCallbackScope::AsyncCallbackScope(Environment* env)
197201
: env_(env) {
198202
env_->makecallback_cntr_++;
@@ -456,17 +460,12 @@ inline double Environment::trigger_async_id() {
456460
inline double Environment::get_default_trigger_async_id() {
457461
double default_trigger_async_id =
458462
async_hooks()->async_id_fields()[AsyncHooks::kDefaultTriggerAsyncId];
459-
async_hooks()->async_id_fields()[AsyncHooks::kDefaultTriggerAsyncId] = -1;
460463
// If defaultTriggerAsyncId isn't set, use the executionAsyncId
461464
if (default_trigger_async_id < 0)
462465
default_trigger_async_id = execution_async_id();
463466
return default_trigger_async_id;
464467
}
465468

466-
inline void Environment::set_default_trigger_async_id(const double id) {
467-
async_hooks()->async_id_fields()[AsyncHooks::kDefaultTriggerAsyncId] = id;
468-
}
469-
470469
inline double* Environment::heap_statistics_buffer() const {
471470
CHECK_NE(heap_statistics_buffer_, nullptr);
472471
return heap_statistics_buffer_;

src/env.h

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -402,22 +402,23 @@ class Environment {
402402
inline size_t stack_size();
403403
inline void clear_async_id_stack(); // Used in fatal exceptions.
404404

405-
// Used to propagate the trigger_async_id to the constructor of any newly
406-
// created resources using RAII. Instead of needing to pass the
407-
// trigger_async_id along with other constructor arguments.
408-
class InitScope {
405+
// Used to set the kDefaultTriggerAsyncId in a scope. This is instead of
406+
// passing the trigger_async_id along with other constructor arguments.
407+
class DefaultTriggerAsyncIdScope {
409408
public:
410-
InitScope() = delete;
411-
explicit InitScope(Environment* env, double init_trigger_async_id);
412-
~InitScope();
409+
DefaultTriggerAsyncIdScope() = delete;
410+
explicit DefaultTriggerAsyncIdScope(Environment* env,
411+
double init_trigger_async_id);
412+
~DefaultTriggerAsyncIdScope();
413413

414414
private:
415-
Environment* env_;
416415
AliasedBuffer<double, v8::Float64Array> async_id_fields_ref_;
416+
double old_default_trigger_async_id_;
417417

418-
DISALLOW_COPY_AND_ASSIGN(InitScope);
418+
DISALLOW_COPY_AND_ASSIGN(DefaultTriggerAsyncIdScope);
419419
};
420420

421+
421422
private:
422423
friend class Environment; // So we can call the constructor.
423424
inline explicit AsyncHooks(v8::Isolate* isolate);
@@ -559,7 +560,6 @@ class Environment {
559560
inline double execution_async_id();
560561
inline double trigger_async_id();
561562
inline double get_default_trigger_async_id();
562-
inline void set_default_trigger_async_id(const double id);
563563

564564
// List of id's that have been destroyed and need the destroy() cb called.
565565
inline std::vector<double>* destroy_async_id_list();

src/pipe_wrap.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,8 @@ Local<Object> PipeWrap::Instantiate(Environment* env,
5353
AsyncWrap* parent,
5454
PipeWrap::SocketType type) {
5555
EscapableHandleScope handle_scope(env->isolate());
56-
AsyncHooks::InitScope init_scope(env, parent->get_async_id());
56+
AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(env,
57+
parent->get_async_id());
5758
CHECK_EQ(false, env->pipe_constructor_template().IsEmpty());
5859
Local<Function> constructor = env->pipe_constructor_template()->GetFunction();
5960
CHECK_EQ(false, constructor.IsEmpty());

0 commit comments

Comments
 (0)