Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 381aef8

Browse filesBrowse files
erinishimotichacjihrig
authored andcommitted
timers: fix cleanup of nested same-timeout timers
For nested timers with the same timeout, we can get into a situation where we have recreated a timer list immediately before we need to clean up an old timer list with the same key. Fix: make sure the list to be deleted is the same instance as the list whose reference was used to determine that a cleanup is necessary. If it's not the same instance, a new list with the same key has been created, and it should not be deleted. Fixes: #7722 PR-URL: #7827 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
1 parent 8d8d70d commit 381aef8
Copy full SHA for 381aef8

File tree

Expand file treeCollapse file tree

2 files changed

+86
-2
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+86
-2
lines changed
Open diff view settings
Collapse file

‎lib/timers.js‎

Copy file name to clipboardExpand all lines: lib/timers.js
+6-2Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -211,9 +211,13 @@ function listOnTimeout() {
211211
debug('%d list empty', msecs);
212212
assert(L.isEmpty(list));
213213
this.close();
214-
if (list._unrefed === true) {
214+
215+
// Either refedLists[msecs] or unrefedLists[msecs] may have been removed and
216+
// recreated since the reference to `list` was created. Make sure they're
217+
// the same instance of the list before destroying.
218+
if (list._unrefed === true && list === unrefedLists[msecs]) {
215219
delete unrefedLists[msecs];
216-
} else {
220+
} else if (list === refedLists[msecs]) {
217221
delete refedLists[msecs];
218222
}
219223
}
Collapse file
+80Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
'use strict';
2+
3+
/*
4+
* This is a regression test for https://github.com/nodejs/node/issues/7722.
5+
*
6+
* When nested timers have the same timeout, calling clearTimeout on the
7+
* older timer after it has fired causes the list the newer timer is in
8+
* to be deleted. Since the newer timer was not cleared, it still blocks
9+
* the event loop completing for the duration of its timeout, however, since
10+
* no reference exists to it in its list, it cannot be canceled and its
11+
* callback is not called when the timeout elapses.
12+
*/
13+
14+
const common = require('../common');
15+
const assert = require('assert');
16+
const Timer = process.binding('timer_wrap').Timer;
17+
18+
const TIMEOUT = common.platformTimeout(100);
19+
const start = Timer.now();
20+
21+
// This bug also prevents the erroneously dereferenced timer's callback
22+
// from being called, so we can't use it's execution or lack thereof
23+
// to assert that the bug is fixed.
24+
process.on('exit', function() {
25+
const end = Timer.now();
26+
assert.equal(end - start < TIMEOUT * 2, true,
27+
'Elapsed time does not include second timer\'s timeout.');
28+
});
29+
30+
const handle1 = setTimeout(common.mustCall(function() {
31+
// Cause the old TIMEOUT list to be deleted
32+
clearTimeout(handle1);
33+
34+
// Cause a new list with the same key (TIMEOUT) to be created for this timer
35+
const handle2 = setTimeout(function() {
36+
common.fail('Inner callback is not called');
37+
}, TIMEOUT);
38+
39+
setTimeout(common.mustCall(function() {
40+
// Attempt to cancel the second timer. Fix for this bug will keep the
41+
// newer timer from being dereferenced by keeping its list from being
42+
// erroneously deleted. If we are able to cancel the timer successfully,
43+
// the bug is fixed.
44+
clearTimeout(handle2);
45+
setImmediate(common.mustCall(function() {
46+
setImmediate(common.mustCall(function() {
47+
const activeHandles = process._getActiveHandles();
48+
const activeTimers = activeHandles.filter(function(handle) {
49+
return handle instanceof Timer;
50+
});
51+
52+
// Make sure our clearTimeout succeeded. One timer finished and
53+
// the other was canceled, so none should be active.
54+
assert.equal(activeTimers.length, 0, 'No Timers remain.');
55+
}));
56+
}));
57+
}), 10);
58+
59+
// Make sure our timers got added to the list.
60+
const activeHandles = process._getActiveHandles();
61+
const activeTimers = activeHandles.filter(function(handle) {
62+
return handle instanceof Timer;
63+
});
64+
const shortTimer = activeTimers.find(function(handle) {
65+
return handle._list.msecs === 10;
66+
});
67+
const longTimers = activeTimers.filter(function(handle) {
68+
return handle._list.msecs === TIMEOUT;
69+
});
70+
71+
// Make sure our clearTimeout succeeded. One timer finished and
72+
// the other was canceled, so none should be active.
73+
assert.equal(activeTimers.length, 3, 'There are 3 timers in the list.');
74+
assert(shortTimer instanceof Timer, 'The shorter timer is in the list.');
75+
assert.equal(longTimers.length, 2, 'Both longer timers are in the list.');
76+
77+
// When this callback completes, `listOnTimeout` should now look at the
78+
// correct list and refrain from removing the new TIMEOUT list which
79+
// contains the reference to the newer timer.
80+
}), TIMEOUT);

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.