Skip to content

Commit 24dd92e

Browse files
Fishrock123Fishrock123
committed
net: use actual Timeout instance on Sockets
This makes `net.Sockets` use actual Timeout objects in a `[kTimeout]` symbol property, rather than making the socket itself a timer and appending properties to it directly. This should make the code generally easier to understand, and might also prevent some deopts from properties being changes on the socket itself. Also moves the Timeout constructor into an internal module. PR-URL: #17704 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
1 parent d6b1b84 commit 24dd92e

File tree

7 files changed

+178
-60
lines changed

7 files changed

+178
-60
lines changed

‎lib/internal/timers.js‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
'use strict';
2+
3+
const async_wrap = process.binding('async_wrap');
4+
// Two arrays that share state between C++ and JS.
5+
const { async_hook_fields, async_id_fields } = async_wrap;
6+
const {
7+
getDefaultTriggerAsyncId,
8+
// The needed emit*() functions.
9+
emitInit
10+
} = require('internal/async_hooks');
11+
// Grab the constants necessary for working with internal arrays.
12+
const { kInit, kAsyncIdCounter } = async_wrap.constants;
13+
// Symbols for storing async id state.
14+
const async_id_symbol = Symbol('asyncId');
15+
const trigger_async_id_symbol = Symbol('triggerId');
16+
17+
const errors = require('internal/errors');
18+
19+
// Timeout values > TIMEOUT_MAX are set to 1.
20+
const TIMEOUT_MAX = 2 ** 31 - 1;
21+
22+
module.exports = {
23+
TIMEOUT_MAX,
24+
kTimeout: Symbol('timeout'), // For hiding Timeouts on other internals.
25+
async_id_symbol,
26+
trigger_async_id_symbol,
27+
Timeout,
28+
setUnrefTimeout,
29+
};
30+
31+
// Timer constructor function.
32+
// The entire prototype is defined in lib/timers.js
33+
function Timeout(callback, after, args, isRepeat) {
34+
after *= 1; // coalesce to number or NaN
35+
if (!(after >= 1 && after <= TIMEOUT_MAX)) {
36+
if (after > TIMEOUT_MAX) {
37+
process.emitWarning(`${after} does not fit into` +
38+
' a 32-bit signed integer.' +
39+
'\nTimeout duration was set to 1.',
40+
'TimeoutOverflowWarning');
41+
}
42+
after = 1; // schedule on next tick, follows browser behavior
43+
}
44+
45+
this._called = false;
46+
this._idleTimeout = after;
47+
this._idlePrev = this;
48+
this._idleNext = this;
49+
this._idleStart = null;
50+
// this must be set to null first to avoid function tracking
51+
// on the hidden class, revisit in V8 versions after 6.2
52+
this._onTimeout = null;
53+
this._onTimeout = callback;
54+
this._timerArgs = args;
55+
this._repeat = isRepeat ? after : null;
56+
this._destroyed = false;
57+
58+
this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter];
59+
this[trigger_async_id_symbol] = getDefaultTriggerAsyncId();
60+
if (async_hook_fields[kInit] > 0) {
61+
emitInit(this[async_id_symbol],
62+
'Timeout',
63+
this[trigger_async_id_symbol],
64+
this);
65+
}
66+
}
67+
68+
var timers;
69+
function getTimers() {
70+
if (timers === undefined) {
71+
timers = require('timers');
72+
}
73+
return timers;
74+
}
75+
76+
function setUnrefTimeout(callback, after, arg1, arg2, arg3) {
77+
// Type checking identical to setTimeout()
78+
if (typeof callback !== 'function') {
79+
throw new errors.TypeError('ERR_INVALID_CALLBACK');
80+
}
81+
82+
let i, args;
83+
switch (arguments.length) {
84+
// fast cases
85+
case 1:
86+
case 2:
87+
break;
88+
case 3:
89+
args = [arg1];
90+
break;
91+
case 4:
92+
args = [arg1, arg2];
93+
break;
94+
default:
95+
args = [arg1, arg2, arg3];
96+
for (i = 5; i < arguments.length; i++) {
97+
// extend array dynamically, makes .apply run much faster in v6.0.0
98+
args[i - 2] = arguments[i];
99+
}
100+
break;
101+
}
102+
103+
const timer = new Timeout(callback, after, args, false);
104+
getTimers()._unrefActive(timer);
105+
106+
return timer;
107+
}

‎lib/net.js‎

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@ var cluster = null;
5757
const errnoException = util._errnoException;
5858
const exceptionWithHostPort = util._exceptionWithHostPort;
5959

60+
const { kTimeout, TIMEOUT_MAX, setUnrefTimeout } = require('internal/timers');
61+
6062
function noop() {}
6163

6264
function createHandle(fd, is_server) {
@@ -201,6 +203,7 @@ function Socket(options) {
201203
this._parent = null;
202204
this._host = null;
203205
this[kLastWriteQueueSize] = 0;
206+
this[kTimeout] = null;
204207

205208
if (typeof options === 'number')
206209
options = { fd: options }; // Legacy interface.
@@ -272,9 +275,12 @@ function Socket(options) {
272275
}
273276
util.inherits(Socket, stream.Duplex);
274277

278+
// Refresh existing timeouts.
275279
Socket.prototype._unrefTimer = function _unrefTimer() {
276-
for (var s = this; s !== null; s = s._parent)
277-
timers._unrefActive(s);
280+
for (var s = this; s !== null; s = s._parent) {
281+
if (s[kTimeout])
282+
timers._unrefActive(s[kTimeout]);
283+
}
278284
};
279285

280286

@@ -387,14 +393,36 @@ Socket.prototype.read = function(n) {
387393
};
388394

389395
Socket.prototype.setTimeout = function(msecs, callback) {
396+
// Type checking identical to timers.enroll()
397+
if (typeof msecs !== 'number') {
398+
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'msecs',
399+
'number', msecs);
400+
}
401+
402+
if (msecs < 0 || !isFinite(msecs)) {
403+
throw new errors.RangeError('ERR_VALUE_OUT_OF_RANGE', 'msecs',
404+
'a non-negative finite number', msecs);
405+
}
406+
407+
// Ensure that msecs fits into signed int32
408+
if (msecs > TIMEOUT_MAX) {
409+
process.emitWarning(`${msecs} does not fit into a 32-bit signed integer.` +
410+
`\nTimer duration was truncated to ${TIMEOUT_MAX}.`,
411+
'TimeoutOverflowWarning');
412+
msecs = TIMEOUT_MAX;
413+
}
414+
415+
// Attempt to clear an existing timer lear in both cases -
416+
// even if it will be rescheduled we don't want to leak an existing timer.
417+
clearTimeout(this[kTimeout]);
418+
390419
if (msecs === 0) {
391-
timers.unenroll(this);
392420
if (callback) {
393421
this.removeListener('timeout', callback);
394422
}
395423
} else {
396-
timers.enroll(this, msecs);
397-
timers._unrefActive(this);
424+
this[kTimeout] = setUnrefTimeout(this._onTimeout.bind(this), msecs);
425+
398426
if (callback) {
399427
this.once('timeout', callback);
400428
}
@@ -551,8 +579,9 @@ Socket.prototype._destroy = function(exception, cb) {
551579

552580
this.readable = this.writable = false;
553581

554-
for (var s = this; s !== null; s = s._parent)
555-
timers.unenroll(s);
582+
for (var s = this; s !== null; s = s._parent) {
583+
clearTimeout(s[kTimeout]);
584+
}
556585

557586
debug('close');
558587
if (this._handle) {

‎lib/timers.js‎

Lines changed: 18 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
const async_wrap = process.binding('async_wrap');
2525
const TimerWrap = process.binding('timer_wrap').Timer;
2626
const L = require('internal/linkedlist');
27+
const timerInternals = require('internal/timers');
2728
const internalUtil = require('internal/util');
2829
const { createPromise, promiseResolve } = process.binding('util');
2930
const assert = require('assert');
@@ -44,8 +45,8 @@ const {
4445
// Grab the constants necessary for working with internal arrays.
4546
const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants;
4647
// Symbols for storing async id state.
47-
const async_id_symbol = Symbol('asyncId');
48-
const trigger_async_id_symbol = Symbol('triggerAsyncId');
48+
const async_id_symbol = timerInternals.async_id_symbol;
49+
const trigger_async_id_symbol = timerInternals.trigger_async_id_symbol;
4950

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

5758
// Timeout values > TIMEOUT_MAX are set to 1.
58-
const TIMEOUT_MAX = 2 ** 31 - 1;
59+
const TIMEOUT_MAX = timerInternals.TIMEOUT_MAX;
60+
61+
// The Timeout class
62+
const Timeout = timerInternals.Timeout;
5963

6064

6165
// HOW and WHY the timers implementation works the way it does.
@@ -446,12 +450,17 @@ function setTimeout(callback, after, arg1, arg2, arg3) {
446450
break;
447451
}
448452

449-
return new Timeout(callback, after, args, false);
453+
const timeout = new Timeout(callback, after, args, false);
454+
active(timeout);
455+
456+
return timeout;
450457
}
451458

452459
setTimeout[internalUtil.promisify.custom] = function(after, value) {
453460
const promise = createPromise();
454-
new Timeout(promise, after, [value], false);
461+
const timeout = new Timeout(promise, after, [value], false);
462+
active(timeout);
463+
455464
return promise;
456465
};
457466

@@ -523,7 +532,10 @@ exports.setInterval = function(callback, repeat, arg1, arg2, arg3) {
523532
break;
524533
}
525534

526-
return new Timeout(callback, repeat, args, true);
535+
const timeout = new Timeout(callback, repeat, args, true);
536+
active(timeout);
537+
538+
return timeout;
527539
};
528540

529541
exports.clearInterval = function(timer) {
@@ -534,44 +546,6 @@ exports.clearInterval = function(timer) {
534546
};
535547

536548

537-
function Timeout(callback, after, args, isRepeat) {
538-
after *= 1; // coalesce to number or NaN
539-
if (!(after >= 1 && after <= TIMEOUT_MAX)) {
540-
if (after > TIMEOUT_MAX) {
541-
process.emitWarning(`${after} does not fit into` +
542-
' a 32-bit signed integer.' +
543-
'\nTimeout duration was set to 1.',
544-
'TimeoutOverflowWarning');
545-
}
546-
after = 1; // schedule on next tick, follows browser behavior
547-
}
548-
549-
this._called = false;
550-
this._idleTimeout = after;
551-
this._idlePrev = this;
552-
this._idleNext = this;
553-
this._idleStart = null;
554-
// this must be set to null first to avoid function tracking
555-
// on the hidden class, revisit in V8 versions after 6.2
556-
this._onTimeout = null;
557-
this._onTimeout = callback;
558-
this._timerArgs = args;
559-
this._repeat = isRepeat ? after : null;
560-
this._destroyed = false;
561-
562-
this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter];
563-
this[trigger_async_id_symbol] = getDefaultTriggerAsyncId();
564-
if (async_hook_fields[kInit] > 0) {
565-
emitInit(this[async_id_symbol],
566-
'Timeout',
567-
this[trigger_async_id_symbol],
568-
this);
569-
}
570-
571-
active(this);
572-
}
573-
574-
575549
function unrefdHandle() {
576550
// Don't attempt to call the callback if it is not a function.
577551
if (typeof this.owner._onTimeout === 'function') {

‎node.gyp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@
123123
'lib/internal/repl/await.js',
124124
'lib/internal/socket_list.js',
125125
'lib/internal/test/unicode.js',
126+
'lib/internal/timers.js',
126127
'lib/internal/tls.js',
127128
'lib/internal/trace_events_async_hooks.js',
128129
'lib/internal/url.js',

‎test/parallel/test-http-client-timeout-on-connect.js‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
1+
// Flags: --expose-internals
2+
13
'use strict';
4+
25
const common = require('../common');
36
const assert = require('assert');
47
const http = require('http');
8+
const { kTimeout } = require('internal/timers');
59

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

1418
req.setTimeout(1);
1519
req.on('socket', common.mustCall((socket) => {
16-
assert.strictEqual(socket._idleTimeout, undefined);
20+
assert.strictEqual(socket[kTimeout], null);
1721
socket.on('connect', common.mustCall(() => {
18-
assert.strictEqual(socket._idleTimeout, 1);
22+
assert.strictEqual(socket[kTimeout]._idleTimeout, 1);
1923
}));
2024
}));
2125
req.on('timeout', common.mustCall(() => req.abort()));

‎test/parallel/test-net-socket-timeout.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,13 @@ const validDelays = [0, 0.001, 1, 1e6];
3636
for (let i = 0; i < nonNumericDelays.length; i++) {
3737
assert.throws(function() {
3838
s.setTimeout(nonNumericDelays[i], () => {});
39-
}, TypeError);
39+
}, TypeError, nonNumericDelays[i]);
4040
}
4141

4242
for (let i = 0; i < badRangeDelays.length; i++) {
4343
assert.throws(function() {
4444
s.setTimeout(badRangeDelays[i], () => {});
45-
}, RangeError);
45+
}, RangeError, badRangeDelays[i]);
4646
}
4747

4848
for (let i = 0; i < validDelays.length; i++) {

‎test/parallel/test-tls-wrap-timeout.js‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
1+
// Flags: --expose_internals
2+
13
'use strict';
24
const common = require('../common');
5+
const { kTimeout, TIMEOUT_MAX } = require('internal/timers');
36

47
if (!common.hasCrypto)
58
common.skip('missing crypto');
@@ -30,13 +33,13 @@ let lastIdleStart;
3033

3134
server.listen(0, () => {
3235
socket = net.connect(server.address().port, function() {
33-
const s = socket.setTimeout(Number.MAX_VALUE, function() {
36+
const s = socket.setTimeout(TIMEOUT_MAX, function() {
3437
throw new Error('timeout');
3538
});
3639
assert.ok(s instanceof net.Socket);
3740

38-
assert.notStrictEqual(socket._idleTimeout, -1);
39-
lastIdleStart = socket._idleStart;
41+
assert.notStrictEqual(socket[kTimeout]._idleTimeout, -1);
42+
lastIdleStart = socket[kTimeout]._idleStart;
4043

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

4952
process.on('exit', () => {
50-
assert.strictEqual(socket._idleTimeout, -1);
51-
assert(lastIdleStart < socket._idleStart);
53+
assert.strictEqual(socket[kTimeout]._idleTimeout, -1);
54+
assert(lastIdleStart < socket[kTimeout]._idleStart);
5255
});

0 commit comments

Comments
 (0)