From 350cb8d90c4fe7c4cb43f307e34fb8c5273af4cb Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 5 Mar 2018 12:20:48 +0100 Subject: [PATCH] async_hooks,process: remove internalNextTick Instead of having mostly duplicate code in form of internalNextTick, instead use the existing defaultAsyncTriggerIdScope with a slight modification which allows undefined triggerAsyncId to be passed in, which then just triggers the callback with the provided arguments. PR-URL: https://github.com/nodejs/node/pull/19147 Refs: https://github.com/nodejs/node/issues/19104 Reviewed-By: Anna Henningsen Reviewed-By: Andreas Madsen Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater --- lib/_http_agent.js | 10 ++-- lib/_http_client.js | 6 +-- lib/_http_outgoing.js | 28 ++++++----- lib/dgram.js | 6 ++- lib/internal/async_hooks.js | 2 + lib/internal/process/next_tick.js | 33 ------------- lib/net.js | 29 ++++++++---- .../test-internal-nexttick-default-trigger.js | 47 ------------------- .../test-nexttick-default-trigger.js | 28 +++++++++++ .../test-no-assert-when-disabled.js | 9 +--- 10 files changed, 79 insertions(+), 119 deletions(-) delete mode 100644 test/async-hooks/test-internal-nexttick-default-trigger.js create mode 100644 test/async-hooks/test-nexttick-default-trigger.js diff --git a/lib/_http_agent.js b/lib/_http_agent.js index 4d5fc7d2ede323..5a18e40b4f13b8 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -26,7 +26,6 @@ const util = require('util'); const EventEmitter = require('events'); const debug = util.debuglog('http'); const { async_id_symbol } = require('internal/async_hooks').symbols; -const { nextTick } = require('internal/process/next_tick'); // New Agent code. @@ -342,10 +341,7 @@ Agent.prototype.destroy = function destroy() { function handleSocketCreation(request, informRequest) { return function handleSocketCreation_Inner(err, socket) { if (err) { - const asyncId = (socket && socket._handle && socket._handle.getAsyncId) ? - socket._handle.getAsyncId() : - null; - nextTick(asyncId, () => request.emit('error', err)); + process.nextTick(emitErrorNT, request, err); return; } if (informRequest) @@ -355,6 +351,10 @@ function handleSocketCreation(request, informRequest) { }; } +function emitErrorNT(emitter, err) { + emitter.emit('error', err); +} + module.exports = { Agent, globalAgent: new Agent() diff --git a/lib/_http_client.js b/lib/_http_client.js index 3687288ee896e1..8e3d61a75612ca 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -36,9 +36,9 @@ const { const { OutgoingMessage } = require('_http_outgoing'); const Agent = require('_http_agent'); const { Buffer } = require('buffer'); +const { defaultTriggerAsyncIdScope } = require('internal/async_hooks'); const { urlToOptions, searchParamsSymbol } = require('internal/url'); const { outHeadersKey, ondrain } = require('internal/http'); -const { nextTick } = require('internal/process/next_tick'); const { ERR_HTTP_HEADERS_SENT, ERR_INVALID_ARG_TYPE, @@ -563,10 +563,10 @@ function responseKeepAlive(res, req) { socket.once('error', freeSocketErrorListener); // There are cases where _handle === null. Avoid those. Passing null to // nextTick() will call getDefaultTriggerAsyncId() to retrieve the id. - const asyncId = socket._handle ? socket._handle.getAsyncId() : null; + const asyncId = socket._handle ? socket._handle.getAsyncId() : undefined; // Mark this socket as available, AFTER user-added end // handlers have a chance to run. - nextTick(asyncId, emitFreeNT, socket); + defaultTriggerAsyncIdScope(asyncId, process.nextTick, emitFreeNT, socket); } } diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index cd7caae9ae735e..1c2250a4b2d836 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -31,8 +31,10 @@ const common = require('_http_common'); const checkIsHttpToken = common._checkIsHttpToken; const checkInvalidHeaderChar = common._checkInvalidHeaderChar; const { outHeadersKey } = require('internal/http'); -const { async_id_symbol } = require('internal/async_hooks').symbols; -const { nextTick } = require('internal/process/next_tick'); +const { + defaultTriggerAsyncIdScope, + symbols: { async_id_symbol } +} = require('internal/async_hooks'); const { ERR_HTTP_HEADERS_SENT, ERR_HTTP_INVALID_HEADER_VALUE, @@ -272,15 +274,14 @@ function _writeRaw(data, encoding, callback) { this._flushOutput(conn); } else if (!data.length) { if (typeof callback === 'function') { - let socketAsyncId = this.socket[async_id_symbol]; // If the socket was set directly it won't be correctly initialized // with an async_id_symbol. // TODO(AndreasMadsen): @trevnorris suggested some more correct // solutions in: // https://github.com/nodejs/node/pull/14389/files#r128522202 - if (socketAsyncId === undefined) socketAsyncId = null; - - nextTick(socketAsyncId, callback); + defaultTriggerAsyncIdScope(conn[async_id_symbol], + process.nextTick, + callback); } return true; } @@ -633,10 +634,13 @@ OutgoingMessage.prototype.write = function write(chunk, encoding, callback) { function write_(msg, chunk, encoding, callback, fromEnd) { if (msg.finished) { const err = new ERR_STREAM_WRITE_AFTER_END(); - nextTick(msg.socket && msg.socket[async_id_symbol], - writeAfterEndNT.bind(msg), - err, - callback); + const triggerAsyncId = msg.socket ? msg.socket[async_id_symbol] : undefined; + defaultTriggerAsyncIdScope(triggerAsyncId, + process.nextTick, + writeAfterEndNT, + msg, + err, + callback); return true; } @@ -686,8 +690,8 @@ function write_(msg, chunk, encoding, callback, fromEnd) { } -function writeAfterEndNT(err, callback) { - this.emit('error', err); +function writeAfterEndNT(msg, err, callback) { + msg.emit('error', err); if (callback) callback(err); } diff --git a/lib/dgram.js b/lib/dgram.js index 78730635cbe268..b9caeddabe2374 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -44,7 +44,6 @@ const { symbols: { async_id_symbol } } = require('internal/async_hooks'); const { UV_UDP_REUSEADDR } = process.binding('constants').os; -const { nextTick } = require('internal/process/next_tick'); const { UDP, SendWrap } = process.binding('udp_wrap'); @@ -526,7 +525,10 @@ Socket.prototype.close = function(callback) { this._stopReceiving(); this._handle.close(); this._handle = null; - nextTick(this[async_id_symbol], socketCloseNT, this); + defaultTriggerAsyncIdScope(this[async_id_symbol], + process.nextTick, + socketCloseNT, + this); return this; }; diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index 72d74243d8cae5..8c4fd08fd83c0f 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -281,6 +281,8 @@ function clearDefaultTriggerAsyncId() { function defaultTriggerAsyncIdScope(triggerAsyncId, block, ...args) { + if (triggerAsyncId === undefined) + return Reflect.apply(block, null, args); // CHECK(Number.isSafeInteger(triggerAsyncId)) // CHECK(triggerAsyncId > 0) const oldDefaultTriggerAsyncId = async_id_fields[kDefaultTriggerAsyncId]; diff --git a/lib/internal/process/next_tick.js b/lib/internal/process/next_tick.js index 39e5a6f8124c89..7acf266cb7f73d 100644 --- a/lib/internal/process/next_tick.js +++ b/lib/internal/process/next_tick.js @@ -1,8 +1,6 @@ 'use strict'; exports.setup = setupNextTick; -// Will be overwritten when setupNextTick() is called. -exports.nextTick = null; function setupNextTick() { const { @@ -87,9 +85,6 @@ function setupNextTick() { // Needs to be accessible from beyond this scope. process._tickCallback = _tickCallback; - // Set the nextTick() function for internal usage. - exports.nextTick = internalNextTick; - function _tickCallback() { let tock; do { @@ -165,32 +160,4 @@ function setupNextTick() { push(new TickObject(callback, args, getDefaultTriggerAsyncId())); } - - // `internalNextTick()` will not enqueue any callback when the process is - // about to exit since the callback would not have a chance to be executed. - function internalNextTick(triggerAsyncId, callback) { - if (typeof callback !== 'function') - throw new ERR_INVALID_CALLBACK(); - // CHECK(Number.isSafeInteger(triggerAsyncId) || triggerAsyncId === null) - // CHECK(triggerAsyncId > 0 || triggerAsyncId === null) - - if (process._exiting) - return; - - var args; - switch (arguments.length) { - case 2: break; - case 3: args = [arguments[2]]; break; - case 4: args = [arguments[2], arguments[3]]; break; - case 5: args = [arguments[2], arguments[3], arguments[4]]; break; - default: - args = new Array(arguments.length - 2); - for (var i = 2; i < arguments.length; i++) - args[i - 2] = arguments[i]; - } - - if (triggerAsyncId === null) - triggerAsyncId = getDefaultTriggerAsyncId(); - push(new TickObject(callback, args, triggerAsyncId)); - } } diff --git a/lib/net.js b/lib/net.js index 7fa1e20aedaec5..19f5b135891eb2 100644 --- a/lib/net.js +++ b/lib/net.js @@ -52,7 +52,6 @@ const { defaultTriggerAsyncIdScope, symbols: { async_id_symbol } } = require('internal/async_hooks'); -const { nextTick } = require('internal/process/next_tick'); const errors = require('internal/errors'); const { ERR_INVALID_ARG_TYPE, @@ -392,7 +391,7 @@ function writeAfterFIN(chunk, encoding, cb) { // TODO: defer error events consistently everywhere, not just the cb this.emit('error', er); if (typeof cb === 'function') { - nextTick(this[async_id_symbol], cb, er); + defaultTriggerAsyncIdScope(this[async_id_symbol], process.nextTick, cb, er); } } @@ -1069,7 +1068,7 @@ function lookupAndConnect(self, options) { // If host is an IP, skip performing a lookup var addressType = isIP(host); if (addressType) { - nextTick(self[async_id_symbol], function() { + defaultTriggerAsyncIdScope(self[async_id_symbol], process.nextTick, () => { if (self.connecting) defaultTriggerAsyncIdScope( self[async_id_symbol], @@ -1372,7 +1371,11 @@ function setupListenHandle(address, port, addressType, backlog, fd) { var ex = exceptionWithHostPort(err, 'listen', address, port); this._handle.close(); this._handle = null; - nextTick(this[async_id_symbol], emitErrorNT, this, ex); + defaultTriggerAsyncIdScope(this[async_id_symbol], + process.nextTick, + emitErrorNT, + this, + ex); return; } @@ -1383,7 +1386,10 @@ function setupListenHandle(address, port, addressType, backlog, fd) { if (this._unref) this.unref(); - nextTick(this[async_id_symbol], emitListeningNT, this); + defaultTriggerAsyncIdScope(this[async_id_symbol], + process.nextTick, + emitListeningNT, + this); } Server.prototype._listen2 = setupListenHandle; // legacy alias @@ -1590,8 +1596,11 @@ Server.prototype.getConnections = function(cb) { const self = this; function end(err, connections) { - const asyncId = self._handle ? self[async_id_symbol] : null; - nextTick(asyncId, cb, err, connections); + defaultTriggerAsyncIdScope(self[async_id_symbol], + process.nextTick, + cb, + err, + connections); } if (!this._usingWorkers) { @@ -1669,8 +1678,10 @@ Server.prototype._emitCloseIfDrained = function() { return; } - const asyncId = this._handle ? this[async_id_symbol] : null; - nextTick(asyncId, emitCloseNT, this); + defaultTriggerAsyncIdScope(this[async_id_symbol], + process.nextTick, + emitCloseNT, + this); }; diff --git a/test/async-hooks/test-internal-nexttick-default-trigger.js b/test/async-hooks/test-internal-nexttick-default-trigger.js deleted file mode 100644 index ed541868542206..00000000000000 --- a/test/async-hooks/test-internal-nexttick-default-trigger.js +++ /dev/null @@ -1,47 +0,0 @@ -'use strict'; -// Flags: --expose-internals -const common = require('../common'); - -// This tests ensures that the triggerId of both the internal and external -// nextTick function sets the triggerAsyncId correctly. - -const assert = require('assert'); -const async_hooks = require('async_hooks'); -const initHooks = require('./init-hooks'); -const { checkInvocations } = require('./hook-checks'); -const internal = require('internal/process/next_tick'); - -const hooks = initHooks(); -hooks.enable(); - -const rootAsyncId = async_hooks.executionAsyncId(); - -// public -process.nextTick(common.mustCall(function() { - assert.strictEqual(async_hooks.triggerAsyncId(), rootAsyncId); -})); - -// internal default -internal.nextTick(null, common.mustCall(function() { - assert.strictEqual(async_hooks.triggerAsyncId(), rootAsyncId); -})); - -// internal -internal.nextTick(rootAsyncId + 1, common.mustCall(function() { - assert.strictEqual(async_hooks.triggerAsyncId(), rootAsyncId + 1); -})); - -process.on('exit', function() { - hooks.sanityCheck(); - - const as = hooks.activitiesOfTypes('TickObject'); - checkInvocations(as[0], { - init: 1, before: 1, after: 1, destroy: 1 - }, 'when process exits'); - checkInvocations(as[1], { - init: 1, before: 1, after: 1, destroy: 1 - }, 'when process exits'); - checkInvocations(as[2], { - init: 1, before: 1, after: 1, destroy: 1 - }, 'when process exits'); -}); diff --git a/test/async-hooks/test-nexttick-default-trigger.js b/test/async-hooks/test-nexttick-default-trigger.js new file mode 100644 index 00000000000000..1bc93f29737389 --- /dev/null +++ b/test/async-hooks/test-nexttick-default-trigger.js @@ -0,0 +1,28 @@ +'use strict'; +const common = require('../common'); + +// This tests ensures that the triggerId of the nextTick function sets the +// triggerAsyncId correctly. + +const assert = require('assert'); +const async_hooks = require('async_hooks'); +const initHooks = require('./init-hooks'); +const { checkInvocations } = require('./hook-checks'); + +const hooks = initHooks(); +hooks.enable(); + +const rootAsyncId = async_hooks.executionAsyncId(); + +process.nextTick(common.mustCall(function() { + assert.strictEqual(async_hooks.triggerAsyncId(), rootAsyncId); +})); + +process.on('exit', function() { + hooks.sanityCheck(); + + const as = hooks.activitiesOfTypes('TickObject'); + checkInvocations(as[0], { + init: 1, before: 1, after: 1, destroy: 1 + }, 'when process exits'); +}); diff --git a/test/async-hooks/test-no-assert-when-disabled.js b/test/async-hooks/test-no-assert-when-disabled.js index 1d26897ab05225..12742bd5d4f014 100644 --- a/test/async-hooks/test-no-assert-when-disabled.js +++ b/test/async-hooks/test-no-assert-when-disabled.js @@ -1,15 +1,8 @@ 'use strict'; // Flags: --no-force-async-hooks-checks --expose-internals -const common = require('../common'); +require('../common'); const async_hooks = require('internal/async_hooks'); -const internal = require('internal/process/next_tick'); - -// Using undefined as the triggerAsyncId. -// Ref: https://github.com/nodejs/node/issues/14386 -// Ref: https://github.com/nodejs/node/issues/14381 -// Ref: https://github.com/nodejs/node/issues/14368 -internal.nextTick(undefined, common.mustCall()); // Negative asyncIds and invalid type name async_hooks.emitInit(-1, null, -1, {});