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.

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

Jump to bottom

lib: simplify nextTick() usage #1612

Closed
wants to merge 1 commit into from
Closed

lib: simplify nextTick() usage #1612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 3 additions & 9 deletions lib/_stream_writable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,22 +293,16 @@ function doWrite(stream, state, writev, len, chunk, encoding, cb) {
}

function onwriteError(stream, state, sync, er, cb) {
--state.pendingcb;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's correct to decrement the counter upfront. Maybe e.g. @chrisdickinson can chime in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure either, but all tests pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, so while I think this is correct, it feels unsafe to decouple pendingcb from actually calling the callback. We end up relying on onwrite to nextTick the next callback in the queue to ensure the order of events happens as planned.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused how this isn't a change functionality? It went from decrementing only in the else to always.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was also being decremented in the nextTick callback.

if (sync)
process.nextTick(onwriteErrorNT, state, cb, er);
else {
state.pendingcb--;
process.nextTick(cb, er);
else
cb(er);
}

stream._writableState.errorEmitted = true;
stream.emit('error', er);
}

function onwriteErrorNT(state, cb, er) {
state.pendingcb--;
cb(er);
}

function onwriteStateUpdate(state) {
state.writing = false;
state.writecb = null;
Expand Down
6 changes: 3 additions & 3 deletions lib/_tls_legacy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,16 +448,16 @@ CryptoStream.prototype.destroy = function(err) {
}
this._opposite.destroy();

process.nextTick(destroyNT, this, err);
process.nextTick(destroyNT, this, err ? true : false);
};


function destroyNT(self, err) {
function destroyNT(self, hadErr) {
// Force EOF
self.push(null);

// Emit 'close' event
self.emit('close', err ? true : false);
self.emit('close', hadErr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved readability?

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough!

Copy link
Member

Choose a reason for hiding this comment

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

Another reason: keeping an exception object alive for some time means keeping a complex object alive that in turn contains references to other objects. A boolean is much (much!) easier for the GC to scan.

}


Expand Down
4 changes: 1 addition & 3 deletions lib/_tls_wrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,9 +444,7 @@ TLSSocket.prototype.renegotiate = function(options, callback) {
}
if (!this._handle.renegotiate()) {
if (callback) {
process.nextTick(function() {
callback(new Error('Failed to renegotiate'));
});
process.nextTick(callback, new Error('Failed to renegotiate'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Of note, this will change the stacktrace for the Error. Not sure if that's a huge problem, since the new stack will be slightly more descriptive.

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH the old error wasn't much better than just throwing a string. At least here you get a pointer to where the error originated.

}
return false;
}
Expand Down
9 changes: 2 additions & 7 deletions lib/dgram.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,19 +321,14 @@ Socket.prototype.send = function(buffer,
!!callback);
if (err && callback) {
// don't emit as error, dgram_legacy.js compatibility
process.nextTick(sendEmitErrorNT, err, address, port, callback);
var ex = exceptionWithHostPort(err, 'send', address, port);
process.nextTick(callback, ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto the stacktrace comment above. Just kidding!

}
}
});
};


function sendEmitErrorNT(err, address, port, callback) {
var ex = exceptionWithHostPort(err, 'send', address, port);
callback(ex);
}


function afterSend(err) {
if (err) {
err = exceptionWithHostPort(err, 'send', this.address, this.port);
Expand Down
11 changes: 5 additions & 6 deletions lib/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,16 @@ function makeAsync(callback) {
// The API already returned, we can invoke the callback immediately.
callback.apply(null, arguments);
} else {
process.nextTick(callMakeAsyncCbNT, callback, arguments);
var args = new Array(arguments.length + 1);
args[0] = callback;
Copy link
Member

Choose a reason for hiding this comment

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

It seems a little bit hacky.
The following code is easy to understand for me.
BUT, I have not bench which code is faster.

var args = [callback];
for (var i = 0; i <arguments.length; i++)
  args.push(arguments[i]);
process.nextTick.apply(null, args);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's faster to specify the length up-front since v8 then knows how much to (pre-)allocate.

Here is a jsperf showing the difference for a short array.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! so this change is reasonable for me.
LGTM.

for (var i = 1, a = 0; a < arguments.length; ++i, ++a)
args[i] = arguments[a];
process.nextTick.apply(null, args);
}
};
}
Copy link
Member

Choose a reason for hiding this comment

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

DNS callbacks have either two or three arguments so maybe it's cleaner to write this as:

return function asyncCallback(arg0, arg1, arg2) {
  if (asyncCallback.immediately) {
    if (arguments.length === 3)
      callback(arg0, arg1, arg2);
    else
      callback(arg0, arg1);
  } else {
    if (arguments.length === 3)
      process.nextTick(callback, arg0, arg1, arg2);
    else
      process.nextTick(callback, arg0, arg1);
  }
};

It's still ugly but it avoids allocating an array and going through .apply().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was going to do that initially but I wanted see if I could write dns benchmarks to see what kind of difference it would make. Optimizing this could be a separate PR though I suppose...



function callMakeAsyncCbNT(callback, args) {
callback.apply(null, args);
}


function onlookup(err, addresses) {
if (err) {
return this.callback(errnoException(err, 'getaddrinfo', this.hostname));
Expand Down
48 changes: 22 additions & 26 deletions lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,14 +268,10 @@ function writeAfterFIN(chunk, encoding, cb) {
// TODO: defer error events consistently everywhere, not just the cb
self.emit('error', er);
if (typeof cb === 'function') {
process.nextTick(writeAfterFINNT, cb, er);
process.nextTick(cb, er);
}
}

function writeAfterFINNT(cb, er) {
cb(er);
}

exports.Socket = Socket;
exports.Stream = Socket; // Legacy naming.

Expand Down Expand Up @@ -426,9 +422,7 @@ Socket.prototype._destroy = function(exception, cb) {
function fireErrorCallbacks() {
if (cb) cb(exception);
if (exception && !self._writableState.errorEmitted) {
process.nextTick(function() {
self.emit('error', exception);
});
process.nextTick(emitErrorNT, self, exception);
self._writableState.errorEmitted = true;
}
};
Expand Down Expand Up @@ -946,7 +940,10 @@ function lookupAndConnect(self, options) {
// immediately calls net.Socket.connect() on it (that's us).
// There are no event listeners registered yet so defer the
// error event to the next tick.
process.nextTick(connectErrorNT, self, err, options);
err.host = options.host;
err.port = options.port;
err.message = err.message + ' ' + options.host + ':' + options.port;
process.nextTick(connectErrorNT, self, err);
} else {
self._unrefTimer();
connect(self,
Expand All @@ -960,10 +957,7 @@ function lookupAndConnect(self, options) {
}


function connectErrorNT(self, err, options) {
err.host = options.host;
err.port = options.port;
err.message = err.message + ' ' + options.host + ':' + options.port;
function connectErrorNT(self, err) {
self.emit('error', err);
self._destroy();
}
Expand Down Expand Up @@ -1177,9 +1171,7 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) {

if (typeof rval === 'number') {
var error = exceptionWithHostPort(rval, 'listen', address, port);
process.nextTick(function() {
self.emit('error', error);
});
process.nextTick(emitErrorNT, self, error);
return;
}
self._handle = rval;
Expand All @@ -1194,9 +1186,7 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) {
var ex = exceptionWithHostPort(err, 'listen', address, port);
self._handle.close();
self._handle = null;
process.nextTick(function() {
self.emit('error', ex);
});
process.nextTick(emitErrorNT, self, ex);
return;
}

Expand All @@ -1211,6 +1201,11 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) {
};


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


function emitListeningNT(self) {
// ensure handle hasn't closed
if (self._handle)
Expand Down Expand Up @@ -1384,9 +1379,7 @@ function onconnection(err, clientHandle) {

Server.prototype.getConnections = function(cb) {
function end(err, connections) {
process.nextTick(function() {
cb(err, connections);
});
process.nextTick(cb, err, connections);
}

if (!this._usingSlaves) {
Expand Down Expand Up @@ -1465,13 +1458,16 @@ Server.prototype._emitCloseIfDrained = function() {
return;
}

process.nextTick(function() {
debug('SERVER: emit close');
self.emit('close');
});
process.nextTick(emitCloseNT, self);
};


function emitCloseNT(self) {
debug('SERVER: emit close');
self.emit('close');
}


Server.prototype.listenFD = util.deprecate(function(fd, type) {
return this.listen({ fd: fd });
}, 'listenFD is deprecated. Use listen({fd: <number>}).');
Expand Down
9 changes: 6 additions & 3 deletions lib/timers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,7 @@ function listOnTimeout() {
// when the timeout threw its exception.
var oldDomain = process.domain;
process.domain = null;
process.nextTick(function() {
list[kOnTimeout]();
});
process.nextTick(listOnTimeoutNT, list);
process.domain = oldDomain;
}
}
Expand All @@ -113,6 +111,11 @@ function listOnTimeout() {
}


function listOnTimeoutNT(list) {
list[kOnTimeout]();
}


const unenroll = exports.unenroll = function(item) {
L.remove(item);

Expand Down