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

net: use actual Timeout instances on Sockets #11154

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
83 changes: 83 additions & 0 deletions lib/internal/timers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
'use strict';

const async_wrap = process.binding('async_wrap');
// Two arrays that share state between C++ and JS.
const { async_hook_fields, async_id_fields } = async_wrap;
const {
initTriggerId,
// The needed emit*() functions.
emitInit
} = require('internal/async_hooks');
// Grab the constants necessary for working with internal arrays.
const { kInit, kAsyncIdCounter } = async_wrap.constants;
// Symbols for storing async id state.
const async_id_symbol = Symbol('asyncId');
const trigger_async_id_symbol = Symbol('triggerId');

const errors = require('internal/errors');

// Timeout values > TIMEOUT_MAX are set to 1.
const TIMEOUT_MAX = 2147483647; // 2^31-1

module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be placed at the bottom of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? It works fine here, and other files have it like that. I don't see a good reason to change?

Copy link
Contributor

@mscdex mscdex Mar 23, 2017

Choose a reason for hiding this comment

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

I believe most of the other files have it at the bottom for a couple of reasons: it's easier to find and it works for situations where you are exporting something whose value is only set later on in the file. Having them all in the same location in each file helps with consistency.

TIMEOUT_MAX,
kTimeout: Symbol('timeout'), // For hiding Timeouts on other internals.
async_id_symbol,
trigger_async_id_symbol,
Timeout,
setUnrefTimeout,
};

// Timer constructor function.
// The entire prototype is defined in lib/timers.js
function Timeout(after, callback, args) {
this._called = false;
this._idleTimeout = after;
this._idlePrev = this;
this._idleNext = this;
this._idleStart = null;
this._onTimeout = callback;
this._timerArgs = args;
this._repeat = null;
this._destroyed = false;
this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter];
this[trigger_async_id_symbol] = initTriggerId();
if (async_hook_fields[kInit] > 0)
emitInit(
this[async_id_symbol], 'Timeout', this[trigger_async_id_symbol], this
);
}

var timers;
function getTimers() {
if (timers === undefined) {
timers = require('timers');
}
return timers;
}

function setUnrefTimeout(callback, after) {
// Type checking identical to setTimeout()
if (typeof callback !== 'function') {
throw new errors.TypeError('ERR_INVALID_CALLBACK');
}

after *= 1; // coalesce to number or NaN
if (!(after >= 1 && after <= TIMEOUT_MAX)) {
if (after > TIMEOUT_MAX) {
process.emitWarning(`${after} does not fit into` +
' a 32-bit signed integer.' +
'\nTimeout duration was set to 1.',
'TimeoutOverflowWarning');
}
after = 1; // schedule on next tick, follows browser behavior
}

const timer = new Timeout(after, callback, null);
if (process.domain)
timer.domain = process.domain;

getTimers()._unrefActive(timer);

return timer;
}
43 changes: 36 additions & 7 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ var cluster = null;
const errnoException = util._errnoException;
const exceptionWithHostPort = util._exceptionWithHostPort;

const { kTimeout, TIMEOUT_MAX, setUnrefTimeout } = require('internal/timers');

function noop() {}

function createHandle(fd) {
Expand Down Expand Up @@ -188,6 +190,7 @@ function Socket(options) {
this._handle = null;
this._parent = null;
this._host = null;
this[kTimeout] = null;

if (typeof options === 'number')
options = { fd: options }; // Legacy interface.
Expand Down Expand Up @@ -259,9 +262,12 @@ function Socket(options) {
}
util.inherits(Socket, stream.Duplex);

// Refresh existing timeouts.
Copy link
Member

Choose a reason for hiding this comment

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

In what way does this “refresh” timeouts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If your run active() or _unrefActive() on an existing timer object it removes it from the underlying list and re-appends it to the appropriate timer pool, refreshing the timer. (Same duration but from "now" rather than when it was first scheduled.)

@addaleax Can you think of a wording that would be more helpful based on that?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks – I think the comment is fine, maybe _unrefActive() just isn’t a very descriptive name.

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 would agree.

Socket.prototype._unrefTimer = function _unrefTimer() {
for (var s = this; s !== null; s = s._parent)
timers._unrefActive(s);
for (var s = this; s !== null; s = s._parent) {
if (s[kTimeout])
timers._unrefActive(s[kTimeout]);
}
};

// the user has called .end(), and all the bytes have been
Expand Down Expand Up @@ -380,14 +386,36 @@ Socket.prototype.listen = function() {


Socket.prototype.setTimeout = function(msecs, callback) {
// Type checking identical to timers.enroll()
if (typeof msecs !== 'number') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'msecs',
'number', msecs);
}

if (msecs < 0 || !isFinite(msecs)) {
throw new errors.RangeError('ERR_VALUE_OUT_OF_RANGE', 'msecs',
'a non-negative finite number', msecs);
}

// Ensure that msecs fits into signed int32
if (msecs > TIMEOUT_MAX) {
process.emitWarning(`${msecs} does not fit into a 32-bit signed integer.` +
`\nTimer duration was truncated to ${TIMEOUT_MAX}.`,
'TimeoutOverflowWarning');
msecs = TIMEOUT_MAX;
}

if (msecs === 0) {
timers.unenroll(this);
clearTimeout(this[kTimeout]);

if (callback) {
this.removeListener('timeout', callback);
}
} else {
timers.enroll(this, msecs);
timers._unrefActive(this);
this[kTimeout] = setUnrefTimeout(() => {
this._onTimeout();
}, msecs);

if (callback) {
this.once('timeout', callback);
}
Expand Down Expand Up @@ -542,8 +570,9 @@ Socket.prototype._destroy = function(exception, cb) {

this.readable = this.writable = false;

for (var s = this; s !== null; s = s._parent)
timers.unenroll(s);
for (var s = this; s !== null; s = s._parent) {
clearTimeout(s[kTimeout]);
}

debug('close');
if (this._handle) {
Expand Down
29 changes: 7 additions & 22 deletions lib/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
const async_wrap = process.binding('async_wrap');
const TimerWrap = process.binding('timer_wrap').Timer;
const L = require('internal/linkedlist');
const timerInternals = require('internal/timers');
const internalUtil = require('internal/util');
const { createPromise, promiseResolve } = process.binding('util');
const assert = require('assert');
Expand All @@ -44,8 +45,8 @@ const {
// Grab the constants necessary for working with internal arrays.
const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants;
// Symbols for storing async id state.
const async_id_symbol = Symbol('asyncId');
const trigger_async_id_symbol = Symbol('triggerAsyncId');
const async_id_symbol = timerInternals.async_id_symbol;
const trigger_async_id_symbol = timerInternals.trigger_async_id_symbol;

/* This is an Uint32Array for easier sharing with C++ land. */
const scheduledImmediateCount = process._scheduledImmediateCount;
Expand All @@ -55,7 +56,10 @@ const activateImmediateCheck = process._activateImmediateCheck;
delete process._activateImmediateCheck;

// Timeout values > TIMEOUT_MAX are set to 1.
const TIMEOUT_MAX = 2147483647; // 2^31-1
const TIMEOUT_MAX = timerInternals.TIMEOUT_MAX; // 2^31-1

// The Timeout class
const Timeout = timerInternals.Timeout;


// HOW and WHY the timers implementation works the way it does.
Expand Down Expand Up @@ -580,25 +584,6 @@ exports.clearInterval = function(timer) {
};


function Timeout(after, callback, args) {
this._called = false;
this._idleTimeout = after;
this._idlePrev = this;
this._idleNext = this;
this._idleStart = null;
this._onTimeout = callback;
this._timerArgs = args;
this._repeat = null;
this._destroyed = false;
this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter];
this[trigger_async_id_symbol] = initTriggerId();
if (async_hook_fields[kInit] > 0)
emitInit(
this[async_id_symbol], 'Timeout', this[trigger_async_id_symbol], this
);
}


function unrefdHandle() {
// Don't attempt to call the callback if it is not a function.
if (typeof this.owner._onTimeout === 'function') {
Expand Down
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@
'lib/internal/repl/await.js',
'lib/internal/socket_list.js',
'lib/internal/test/unicode.js',
'lib/internal/timers.js',
'lib/internal/tls.js',
'lib/internal/trace_events_async_hooks.js',
'lib/internal/url.js',
Expand Down
8 changes: 6 additions & 2 deletions test/parallel/test-http-client-timeout-on-connect.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
// Flags: --expose-internals

'use strict';

const common = require('../common');
const assert = require('assert');
const http = require('http');
const { kTimeout } = require('internal/timers');

const server = http.createServer((req, res) => {
// This space is intentionally left blank.
Expand All @@ -13,9 +17,9 @@ server.listen(0, common.localhostIPv4, common.mustCall(() => {

req.setTimeout(1);
req.on('socket', common.mustCall((socket) => {
assert.strictEqual(socket._idleTimeout, undefined);
assert.strictEqual(socket[kTimeout], null);
socket.on('connect', common.mustCall(() => {
assert.strictEqual(socket._idleTimeout, 1);
assert.strictEqual(socket[kTimeout]._idleTimeout, 1);
}));
}));
req.on('timeout', common.mustCall(() => req.abort()));
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-net-socket-timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,13 @@ const validDelays = [0, 0.001, 1, 1e6];
for (let i = 0; i < nonNumericDelays.length; i++) {
assert.throws(function() {
s.setTimeout(nonNumericDelays[i], () => {});
}, TypeError);
}, TypeError, nonNumericDelays[i]);
}

for (let i = 0; i < badRangeDelays.length; i++) {
assert.throws(function() {
s.setTimeout(badRangeDelays[i], () => {});
}, RangeError);
}, RangeError, badRangeDelays[i]);
}

for (let i = 0; i < validDelays.length; i++) {
Expand Down
13 changes: 8 additions & 5 deletions test/parallel/test-tls-wrap-timeout.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
// Flags: --expose_internals

'use strict';
const common = require('../common');
const { kTimeout, TIMEOUT_MAX } = require('internal/timers');

if (!common.hasCrypto)
common.skip('missing crypto');
Expand Down Expand Up @@ -30,13 +33,13 @@ let lastIdleStart;

server.listen(0, () => {
socket = net.connect(server.address().port, function() {
const s = socket.setTimeout(Number.MAX_VALUE, function() {
const s = socket.setTimeout(TIMEOUT_MAX, function() {
throw new Error('timeout');
});
assert.ok(s instanceof net.Socket);

assert.notStrictEqual(socket._idleTimeout, -1);
lastIdleStart = socket._idleStart;
assert.notStrictEqual(socket[kTimeout]._idleTimeout, -1);
lastIdleStart = socket[kTimeout]._idleStart;

const tsocket = tls.connect({
socket: socket,
Expand All @@ -47,6 +50,6 @@ server.listen(0, () => {
});

process.on('exit', () => {
assert.strictEqual(socket._idleTimeout, -1);
assert(lastIdleStart < socket._idleStart);
assert.strictEqual(socket[kTimeout]._idleTimeout, -1);
assert(lastIdleStart < socket[kTimeout]._idleStart);
});
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ server.listen(0, common.mustCall(() => {
server.close();
}));

server.on('clientError', (e) => console.error(e));

function request(callback) {
socket.setEncoding('utf8');
socket.on('data', onData);
Expand All @@ -49,6 +51,7 @@ server.listen(0, common.mustCall(() => {
}

function onHeaders() {
console.log(require('util').inspect(response));
assert.ok(response.includes('HTTP/1.1 200 OK\r\n'));
assert.ok(response.includes('Connection: keep-alive\r\n'));
callback();
Expand Down