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.

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

async_hooks,process: remove internalNextTick #19147

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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) ?
Copy link
Contributor Author

@apapirovski apapirovski Mar 5, 2018

Choose a reason for hiding this comment

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

After checking all the call sites, it's not possible to receive a socket here if err is truthy. (I've also double-checked this by adding an assert during debugging.)

socket._handle.getAsyncId() :
null;
nextTick(asyncId, () => request.emit('error', err));
process.nextTick(emitErrorNT, request, err);
return;
}
if (informRequest)
Expand All @@ -355,6 +351,10 @@ function handleSocketCreation(request, informRequest) {
};
}

function emitErrorNT(emitter, err) {
emitter.emit('error', err);
}

module.exports = {
Agent,
globalAgent: new Agent()
Expand Down
6 changes: 3 additions & 3 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
}

Expand Down
28 changes: 16 additions & 12 deletions lib/_http_outgoing.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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],
Copy link
Member

Choose a reason for hiding this comment

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

Is conn[async_id_symbol] === this.socket[async_id_symbol]? Also, this changes the socketAsyncId so if this is correct the comment should be updated too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

conn === this.socket so it's equivalent. Not quite clear on the second part of the feedback — the comment above it still applies.

process.nextTick,
callback);
}
return true;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
}

Expand Down
6 changes: 4 additions & 2 deletions lib/dgram.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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;
};
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
33 changes: 0 additions & 33 deletions lib/internal/process/next_tick.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
'use strict';

exports.setup = setupNextTick;
// Will be overwritten when setupNextTick() is called.
exports.nextTick = null;

function setupNextTick() {
const {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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));
}
}
29 changes: 20 additions & 9 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -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;
}

Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
};


Expand Down
47 changes: 0 additions & 47 deletions test/async-hooks/test-internal-nexttick-default-trigger.js

This file was deleted.

28 changes: 28 additions & 0 deletions test/async-hooks/test-nexttick-default-trigger.js
Original file line number Diff line number Diff line change
@@ -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');
});
9 changes: 1 addition & 8 deletions test/async-hooks/test-no-assert-when-disabled.js
Original file line number Diff line number Diff line change
@@ -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, {});
Expand Down