test: add test-domain-exit-dispose-again back

1c85849 "fixed" test-domain-exit-dispose-again by changing its logic to test that process.domain was cleared properly in case an error was thrown from a timer's callback. However, it became clear when reviewing a recent change that refactors lib/timers.js that it was not quite the intention of the original test. Thus, this change adds the original implementation of test-domain-exit-dispose-again back, with comments that make its implementation easier to understand. It also preserve the changes made by 1c85849, but it moves them to a new test file named test-timers-reset-process-domain-on-throw.js. PR: #4256 PR-URL: #4256 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
nodejs · Fishrock123 · Jan 6, 2016 · Dec 17, 2015 · Dec 17, 2015 · Dec 14, 2015
commit 2a69ab32ece382a4ae3f4ba527ec5340f37501f4
diff --git a/test/parallel/test-domain-exit-dispose-again.js b/test/parallel/test-domain-exit-dispose-again.js
@@ -1,39 +1,76 @@
 'use strict';
-const common = require('../common');
-const assert = require('assert');
-const domain = require('domain');
 
-// Use the same timeout value so that both timers' callbacks are called during
-// the same invocation of the underlying native timer's callback (listOnTimeout
-// in lib/timers.js).
-setTimeout(err, 50);
-setTimeout(common.mustCall(secondTimer), 50);
+// This test makes sure that when a domain is disposed, timers that are
+// attached to that domain are not fired, but timers that are _not_ attached
+// to that domain, including those whose callbacks are called from within
+// the same invocation of listOnTimeout, _are_ called.
 
-function err() {
+var common = require('../common');
+var assert = require('assert');
+var domain = require('domain');
+var disposalFailed = false;
+
+// Repeatedly schedule a timer with a delay different than the timers attached
+// to a domain that will eventually be disposed to make sure that they are
+// called, regardless of what happens with those timers attached to domains
+// that will eventually be disposed.
+var a = 0;
+log();
+function log() {
+  console.log(a++, process.domain);
+  if (a < 10) setTimeout(log, 20);
+}
+
+var secondTimerRan = false;
+
+// Use the same timeout duration for both "firstTimer" and "secondTimer"
+// callbacks so that they are called during the same invocation of the
+// underlying native timer's callback (listOnTimeout in lib/timers.js).
+const TIMEOUT_DURATION = 50;
+
+setTimeout(function firstTimer() {
   const d = domain.create();
-  d.on('error', handleDomainError);
-  d.run(err2);
 
-  function err2() {
-    // this function doesn't exist, and throws an error as a result.
+  d.on('error', function handleError(err) {
+    // Dispose the domain on purpose, so that we can test that nestedTimer
+    // is not called since it's associated to this domain and a timer whose
+    // domain is diposed should not run.
+    d.dispose();
+    console.error(err);
+    console.error('in domain error handler',
+        process.domain, process.domain === d);
+  });
+
+  d.run(function() {
+    // Create another nested timer that is by definition associated to the
+    // domain "d". Because an error is thrown before the timer's callback
+    // is called, and because the domain's error handler disposes the domain,
+    // this timer's callback should never run.
+    setTimeout(function nestedTimer() {
+      console.error('Nested timer should not run, because it is attached to ' +
+          'a domain that should be disposed.');
+      disposalFailed = true;
+      process.exit(1);
+    });
+
+    // Make V8 throw an unreferenced error. As a result, the domain's error
+    // handler is called, which disposes the domain "d" and should prevent the
+    // nested timer that is attached to it from running.
     err3();
-  }
+  });
+}, TIMEOUT_DURATION);
 
-  function handleDomainError(e) {
-    // In the domain's error handler, the current active domain should be the
-    // domain within which the error was thrown.
-    assert.equal(process.domain, d);
-  }
-}
+// This timer expires in the same invocation of listOnTimeout than firstTimer,
+// but because it's not attached to any domain, it must run regardless of
+// domain "d" being disposed.
+setTimeout(function secondTimer() {
+  console.log('In second timer');
+  secondTimerRan = true;
+}, TIMEOUT_DURATION);
 
-function secondTimer() {
-  // secondTimer was scheduled before any domain had been created, so its
-  // callback should not have any active domain set when it runs.
-  // Do not use assert here, as it throws errors and if a domain with an error
-  // handler is active, then asserting wouldn't make the test fail.
-  if (process.domain !== null) {
-    console.log('process.domain should be null, but instead is:',
-      process.domain);
-    process.exit(1);
-  }
-}
+process.on('exit', function() {
+  assert.equal(a, 10);
+  assert.equal(disposalFailed, false);
+  assert(secondTimerRan);
+  console.log('ok');
+});
diff --git a/test/parallel/test-timers-reset-process-domain-on-throw.js b/test/parallel/test-timers-reset-process-domain-on-throw.js
@@ -0,0 +1,45 @@
+'use strict';
+
+// This test makes sure that when throwing from within a timer's callback,
+// its active domain at the time of the throw is not the process' active domain
+// for the next timers that need to be processed on the same turn of the event
+// loop.
+
+const common = require('../common');
+const assert = require('assert');
+const domain = require('domain');
+
+// Use the same timeout value so that both timers' callbacks are called during
+// the same invocation of the underlying native timer's callback (listOnTimeout
+// in lib/timers.js).
+setTimeout(err, 50);
+setTimeout(common.mustCall(secondTimer), 50);
+
+function err() {
+  const d = domain.create();
+  d.on('error', handleDomainError);
+  d.run(err2);
+
+  function err2() {
+    // this function doesn't exist, and throws an error as a result.
+    err3();
+  }
+
+  function handleDomainError(e) {
+    // In the domain's error handler, the current active domain should be the
+    // domain within which the error was thrown.
+    assert.equal(process.domain, d);
+  }
+}
+
+function secondTimer() {
+  // secondTimer was scheduled before any domain had been created, so its
+  // callback should not have any active domain set when it runs.
+  if (process.domain !== null) {
+    console.log('process.domain should be null in this timer callback, but ' +
+        'instead is:', process.domain);
+    // Do not use assert here, as it throws errors and if a domain with an error
+    // handler is active, then asserting wouldn't make the test fail.
+    process.exit(1);
+  }
+}