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

d1ba82af1c "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 preserves the changes made by
d1ba82af1c, but it moves them to a new
test file named test-timers-reset-process-domain-on-throw.js.

PR: #4278
PR-URL: https://github.com/nodejs/node/pull/4278
Reviewed-By: James M Snell <jasnell@gmail.com>

test/simple/test-domain-exit-dispose-again.js

@@ -19,41 +19,74 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
-var common = require('../common');
+// 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.
+
+var common = require('../common.js');
 var assert = require('assert');
 var domain = require('domain');
+var disposalFailed = false;
 
-// 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);
+// 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);
+}
 
-function err() {
+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).
+var TIMEOUT_DURATION = 50;
+
+setTimeout(function firstTimer() {
   var d = domain.create();
-  d.on('error', handleDomainError);
-  d.run(err2);
+  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 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);
+    });
 
-  function err2() {
-    // this function doesn't exist, and throws an error as a result.
+    // 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');
+});

test/simple/test-timers-reset-process-domain-on-throw.js

@@ -0,0 +1,59 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+var 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() {
+  var 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.
+  // 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);
+  }
+}