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 74ff9bc

Browse filesBrowse files
Fishrock123Fishrock123
committed
timers: minor _unrefActive fixes and improvements
This commit addresses most of the review comments in #2540, which are kept in this separate commit so as to better preserve the prior two patches as they landed in 0.12. This commit: - Fixes a bug with unrefActive timers and disposed domains. - Fixes a bug with unrolling an unrefActive timer from another. - Adds a test for both above bugs. - Improves check logic, making it stricter, simpler, or both. - Optimizes nicer with a smaller, separate function for the try/catch. Fixes: nodejs/node-convergence-archive#23 Ref: #268 PR-URL: #2540 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
1 parent 5d14a6e commit 74ff9bc
Copy full SHA for 74ff9bc

File tree

Expand file treeCollapse file tree

2 files changed

+68
-17
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+68
-17
lines changed
Open diff view settings
Collapse file

‎lib/timers.js‎

Copy file name to clipboardExpand all lines: lib/timers.js
+22-17Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -481,31 +481,36 @@ function _makeTimerTimeout(timer) {
481481
var domain = timer.domain;
482482
var msecs = timer._idleTimeout;
483483

484+
L.remove(timer);
485+
484486
// Timer has been unenrolled by another timer that fired at the same time,
485487
// so don't make it timeout.
486-
if (!msecs || msecs < 0)
488+
if (msecs <= 0)
487489
return;
488490

489491
if (!timer._onTimeout)
490492
return;
491493

492-
if (domain && domain._disposed)
493-
return;
494+
if (domain) {
495+
if (domain._disposed)
496+
return;
494497

495-
try {
496-
var threw = true;
498+
domain.enter();
499+
}
497500

498-
if (domain) domain.enter();
501+
debug('unreftimer firing timeout');
502+
timer._called = true;
503+
_runOnTimeout(timer);
499504

500-
debug('unreftimer firing timeout');
501-
L.remove(timer);
502-
timer._called = true;
503-
timer._onTimeout();
505+
if (domain)
506+
domain.exit();
507+
}
504508

509+
function _runOnTimeout(timer) {
510+
var threw = true;
511+
try {
512+
timer._onTimeout();
505513
threw = false;
506-
507-
if (domain)
508-
domain.exit();
509514
} finally {
510515
if (threw) process.nextTick(unrefTimeout);
511516
}
@@ -519,7 +524,7 @@ function unrefTimeout() {
519524
var timeSinceLastActive;
520525
var nextTimeoutTime;
521526
var nextTimeoutDuration;
522-
var minNextTimeoutTime;
527+
var minNextTimeoutTime = TIMEOUT_MAX;
523528
var timersToTimeout = [];
524529

525530
// The actual timer fired and has not yet been rearmed,
@@ -534,7 +539,7 @@ function unrefTimeout() {
534539
// and rearm the actual timer if the next timeout to expire
535540
// will expire before the current actual timer.
536541
var cur = unrefList._idlePrev;
537-
while (cur != unrefList) {
542+
while (cur !== unrefList) {
538543
timeSinceLastActive = now - cur._idleStart;
539544

540545
if (timeSinceLastActive < cur._idleTimeout) {
@@ -543,7 +548,7 @@ function unrefTimeout() {
543548

544549
nextTimeoutDuration = cur._idleTimeout - timeSinceLastActive;
545550
nextTimeoutTime = now + nextTimeoutDuration;
546-
if (minNextTimeoutTime == null ||
551+
if (minNextTimeoutTime === TIMEOUT_MAX ||
547552
(nextTimeoutTime < minNextTimeoutTime)) {
548553
// We found a timeout that will expire earlier,
549554
// store its next timeout time now so that we
@@ -569,7 +574,7 @@ function unrefTimeout() {
569574

570575
// Rearm the actual timer with the timeout delay
571576
// of the earliest timeout found.
572-
if (minNextTimeoutTime != null) {
577+
if (minNextTimeoutTime !== TIMEOUT_MAX) {
573578
unrefTimer.start(minNextTimeoutTime - now, 0);
574579
unrefTimer.when = minNextTimeoutTime;
575580
debug('unrefTimer rescheduled');
Collapse file

‎test/parallel/test-timers-unref-active-unenrolled-disposed.js‎

Copy file name to clipboard
+46Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
'use strict';
2+
3+
// https://github.com/nodejs/node/pull/2540/files#r38231197
4+
5+
const common = require('../common');
6+
const timers = require('timers');
7+
const assert = require('assert');
8+
const domain = require('domain');
9+
10+
// Crazy stuff to keep the process open,
11+
// then close it when we are actually done.
12+
const TEST_DURATION = common.platformTimeout(100);
13+
const keepOpen = setTimeout(function() {
14+
throw new Error('Test timed out. keepOpen was not canceled.');
15+
}, TEST_DURATION);
16+
17+
const endTest = makeTimer(2);
18+
19+
const someTimer = makeTimer(1);
20+
someTimer.domain = domain.create();
21+
someTimer.domain.dispose();
22+
someTimer._onTimeout = function() {
23+
throw new Error('someTimer was not supposed to fire!');
24+
};
25+
26+
endTest._onTimeout = common.mustCall(function() {
27+
assert.strictEqual(someTimer._idlePrev, null);
28+
assert.strictEqual(someTimer._idleNext, null);
29+
clearTimeout(keepOpen);
30+
});
31+
32+
const cancelsTimer = makeTimer(1);
33+
cancelsTimer._onTimeout = common.mustCall(function() {
34+
someTimer._idleTimeout = 0;
35+
});
36+
37+
timers._unrefActive(cancelsTimer);
38+
timers._unrefActive(someTimer);
39+
timers._unrefActive(endTest);
40+
41+
function makeTimer(msecs) {
42+
const timer = {};
43+
timers.unenroll(timer);
44+
timers.enroll(timer, msecs);
45+
return timer;
46+
}

0 commit comments

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