Permalink
Browse files

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>
  • Loading branch information...
Fishrock123 committed Aug 27, 2015
1 parent 403d7ee commit f6b093343da915832c20bfbc040c7d6bef26f08c
Showing with 68 additions and 17 deletions.
  1. +22 −17 lib/timers.js
  2. +46 −0 test/parallel/test-timers-unref-active-unenrolled-disposed.js
View
@@ -481,31 +481,36 @@ function _makeTimerTimeout(timer) {
var domain = timer.domain;
var msecs = timer._idleTimeout;
+ L.remove(timer);
+
// Timer has been unenrolled by another timer that fired at the same time,
// so don't make it timeout.
- if (!msecs || msecs < 0)
+ if (msecs <= 0)
return;
if (!timer._onTimeout)
return;
- if (domain && domain._disposed)
- return;
+ if (domain) {
+ if (domain._disposed)
+ return;
- try {
- var threw = true;
+ domain.enter();
+ }
- if (domain) domain.enter();
+ debug('unreftimer firing timeout');
+ timer._called = true;
+ _runOnTimeout(timer);
- debug('unreftimer firing timeout');
- L.remove(timer);
- timer._called = true;
- timer._onTimeout();
+ if (domain)
+ domain.exit();
+}
+function _runOnTimeout(timer) {
+ var threw = true;
+ try {
+ timer._onTimeout();
threw = false;
-
- if (domain)
- domain.exit();
} finally {
if (threw) process.nextTick(unrefTimeout);
}
@@ -519,7 +524,7 @@ function unrefTimeout() {
var timeSinceLastActive;
var nextTimeoutTime;
var nextTimeoutDuration;
- var minNextTimeoutTime;
+ var minNextTimeoutTime = TIMEOUT_MAX;
var timersToTimeout = [];
// The actual timer fired and has not yet been rearmed,
@@ -534,7 +539,7 @@ function unrefTimeout() {
// and rearm the actual timer if the next timeout to expire
// will expire before the current actual timer.
var cur = unrefList._idlePrev;
- while (cur != unrefList) {
+ while (cur !== unrefList) {
timeSinceLastActive = now - cur._idleStart;
if (timeSinceLastActive < cur._idleTimeout) {
@@ -543,7 +548,7 @@ function unrefTimeout() {
nextTimeoutDuration = cur._idleTimeout - timeSinceLastActive;
nextTimeoutTime = now + nextTimeoutDuration;
- if (minNextTimeoutTime == null ||
+ if (minNextTimeoutTime === TIMEOUT_MAX ||
(nextTimeoutTime < minNextTimeoutTime)) {
// We found a timeout that will expire earlier,
// store its next timeout time now so that we
@@ -569,7 +574,7 @@ function unrefTimeout() {
// Rearm the actual timer with the timeout delay
// of the earliest timeout found.
- if (minNextTimeoutTime != null) {
+ if (minNextTimeoutTime !== TIMEOUT_MAX) {
unrefTimer.start(minNextTimeoutTime - now, 0);
unrefTimer.when = minNextTimeoutTime;
debug('unrefTimer rescheduled');
@@ -0,0 +1,46 @@
+'use strict';
+
+// https://github.com/nodejs/node/pull/2540/files#r38231197
+
+const common = require('../common');
+const timers = require('timers');
+const assert = require('assert');
+const domain = require('domain');
+
+// Crazy stuff to keep the process open,
+// then close it when we are actually done.
+const TEST_DURATION = common.platformTimeout(100);
+const keepOpen = setTimeout(function() {
+ throw new Error('Test timed out. keepOpen was not canceled.');
+}, TEST_DURATION);
+
+const endTest = makeTimer(2);
+
+const someTimer = makeTimer(1);
+someTimer.domain = domain.create();
+someTimer.domain.dispose();
+someTimer._onTimeout = function() {
+ throw new Error('someTimer was not supposed to fire!');
+};
+
+endTest._onTimeout = common.mustCall(function() {
+ assert.strictEqual(someTimer._idlePrev, null);
+ assert.strictEqual(someTimer._idleNext, null);
+ clearTimeout(keepOpen);
+});
+
+const cancelsTimer = makeTimer(1);
+cancelsTimer._onTimeout = common.mustCall(function() {
+ someTimer._idleTimeout = 0;
+});
+
+timers._unrefActive(cancelsTimer);
+timers._unrefActive(someTimer);
+timers._unrefActive(endTest);
+
+function makeTimer(msecs) {
+ const timer = {};
+ timers.unenroll(timer);
+ timers.enroll(timer, msecs);
+ return timer;
+}

0 comments on commit f6b0933

Please sign in to comment.