domain: remove .dispose()

`domain.dispose()` is generally considered an anti-pattern, has been
runtime-deprecated for over 4 years, and is a part of the `domain`
module that can not be emulated by `async_hooks`; so remove it.

Ref: https://nodejs.org/en/docs/guides/domain-postmortem/
Ref: 4a74fc9776d825115849997f4adacb46f4303494

PR-URL: https://github.com/nodejs/node/pull/15412
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

doc/api/deprecations.md

@@ -146,9 +146,9 @@ instead.
 <a id="DEP0012"></a>
 ### DEP0012: Domain.dispose
 
-Type: Runtime
+Type: End-of-Life
 
-[`Domain.dispose()`][] is deprecated. Recover from failed I/O actions
+[`Domain.dispose()`][] is removed. Recover from failed I/O actions
 explicitly via error event handlers set on the domain instead.
 
 <a id="DEP0013"></a>

doc/api/domain.md

@@ -217,7 +217,7 @@ added.
 
 Implicit binding routes thrown errors and `'error'` events to the
 Domain's `'error'` event, but does not register the EventEmitter on the
-Domain, so [`domain.dispose()`][] will not shut down the EventEmitter.
+Domain.
 Implicit binding only takes care of thrown errors and `'error'` events.
 
 ## Explicit Binding
@@ -329,15 +329,6 @@ d.on('error', (er) => {
 });
 ```
 
-### domain.dispose()
-
-> Stability: 0 - Deprecated.  Please recover from failed IO actions
-> explicitly via error event handlers set on the domain.
-
-Once `dispose` has been called, the domain will no longer be used by callbacks
-bound into the domain via `run`, `bind`, or `intercept`, and a `'dispose'` event
-is emitted.
-
 ### domain.enter()
 
 The `enter` method is plumbing used by the `run`, `bind`, and `intercept`
@@ -351,9 +342,6 @@ Calling `enter` changes only the active domain, and does not alter the domain
 itself. `enter` and `exit` can be called an arbitrary number of times on a
 single domain.
 
-If the domain on which `enter` is called has been disposed, `enter` will return
-without setting the domain.
-
 ### domain.exit()
 
 The `exit` method exits the current domain, popping it off the domain stack.
@@ -369,9 +357,6 @@ Calling `exit` changes only the active domain, and does not alter the domain
 itself. `enter` and `exit` can be called an arbitrary number of times on a
 single domain.
 
-If the domain on which `exit` is called has been disposed, `exit` will return
-without exiting the domain.
-
 ### domain.intercept(callback)
 
 * `callback` {Function} The callback function
@@ -500,7 +485,6 @@ rejections.
 [`EventEmitter`]: events.html#events_class_eventemitter
 [`domain.add(emitter)`]: #domain_domain_add_emitter
 [`domain.bind(callback)`]: #domain_domain_bind_callback
-[`domain.dispose()`]: #domain_domain_dispose
 [`domain.exit()`]: #domain_domain_exit
 [`setInterval()`]: timers.html#timers_setinterval_callback_delay_args
 [`setTimeout()`]: timers.html#timers_settimeout_callback_delay_args

lib/domain.js

@@ -75,21 +75,12 @@ function Domain() {
 }
 
 Domain.prototype.members = undefined;
-Domain.prototype._disposed = undefined;
 
 
 // Called by process._fatalException in case an error was thrown.
 Domain.prototype._errorHandler = function _errorHandler(er) {
   var caught = false;
 
-  // ignore errors on disposed domains.
-  //
-  // XXX This is a bit stupid.  We should probably get rid of
-  // domain.dispose() altogether.  It's almost always a terrible
-  // idea.  --isaacs
-  if (this._disposed)
-    return true;
-
   if (!util.isPrimitive(er)) {
     er.domain = this;
     er.domainThrown = true;
@@ -160,8 +151,6 @@ Domain.prototype._errorHandler = function _errorHandler(er) {
 
 
 Domain.prototype.enter = function() {
-  if (this._disposed) return;
-
   // note that this might be a no-op, but we still need
   // to push it onto the stack so that we can pop it later.
   exports.active = process.domain = this;
@@ -171,10 +160,9 @@ Domain.prototype.enter = function() {
 
 
 Domain.prototype.exit = function() {
-  // skip disposed domains, as usual, but also don't do anything if this
+  // don't do anything if this domain is not on the stack.
-  // domain is not on the stack.
   var index = stack.lastIndexOf(this);
-  if (this._disposed || index === -1) return;
+  if (index === -1) return;
 
   // exit all domains until this one.
   stack.splice(index);
@@ -187,8 +175,8 @@ Domain.prototype.exit = function() {
 
 // note: this works for timers as well.
 Domain.prototype.add = function(ee) {
-  // If the domain is disposed or already added, then nothing left to do.
+  // If the domain is already added, then nothing left to do.
-  if (this._disposed || ee.domain === this)
+  if (ee.domain === this)
     return;
 
   // has a domain already - remove it first.
@@ -224,9 +212,6 @@ Domain.prototype.remove = function(ee) {
 
 
 Domain.prototype.run = function(fn) {
-  if (this._disposed)
-    return;
-
   var ret;
 
   this.enter();
@@ -248,9 +233,6 @@ Domain.prototype.run = function(fn) {
 
 
 function intercepted(_this, self, cb, fnargs) {
-  if (self._disposed)
-    return;
-
   if (fnargs[0] && fnargs[0] instanceof Error) {
     var er = fnargs[0];
     util._extend(er, {
@@ -291,9 +273,6 @@ Domain.prototype.intercept = function(cb) {
 
 
 function bound(_this, self, cb, fnargs) {
-  if (self._disposed)
-    return;
-
   var ret;
 
   self.enter();
@@ -318,22 +297,3 @@ Domain.prototype.bind = function(cb) {
 
   return runBound;
 };
-
-
-Domain.prototype.dispose = util.deprecate(function() {
-  if (this._disposed) return;
-
-  // if we're the active domain, then get out now.
-  this.exit();
-
-  // remove from parent domain, if there is one.
-  if (this.domain) this.domain.remove(this);
-
-  // kill the references so that they can be properly gc'ed.
-  this.members.length = 0;
-
-  // mark this domain as 'no longer relevant'
-  // so that it can't be entered or activated.
-  this._disposed = true;
-}, 'Domain.dispose is deprecated. Recover from failed I/O actions explicitly ' +
-    'via error event handlers set on the domain instead.', 'DEP0012');

lib/timers.js

@@ -250,15 +250,6 @@ function listOnTimeout() {
 
     var domain = timer.domain;
     if (domain) {
-
-      // If the timer callback throws and the
-      // domain or uncaughtException handler ignore the exception,
-      // other timers that expire on this tick should still run.
-      //
-      // https://github.com/nodejs/node-v0.x-archive/issues/2631
-      if (domain._disposed)
-        continue;
-
       domain.enter();
     }
 

src/env.h

@@ -116,7 +116,6 @@ struct performance_state;
   V(dest_string, "dest")                                                      \
   V(destroy_string, "destroy")                                                \
   V(detached_string, "detached")                                              \
-  V(disposed_string, "_disposed")                                             \
   V(dns_a_string, "A")                                                        \
   V(dns_aaaa_string, "AAAA")                                                  \
   V(dns_cname_string, "CNAME")                                                \

src/node.cc

@@ -1159,12 +1159,10 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) {
 }
 
 
-bool DomainEnter(Environment* env, Local<Object> object) {
+void DomainEnter(Environment* env, Local<Object> object) {
   Local<Value> domain_v = object->Get(env->domain_string());
   if (domain_v->IsObject()) {
     Local<Object> domain = domain_v.As<Object>();
-    if (domain->Get(env->disposed_string())->IsTrue())
-      return true;
     Local<Value> enter_v = domain->Get(env->enter_string());
     if (enter_v->IsFunction()) {
       if (enter_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
@@ -1173,16 +1171,13 @@ bool DomainEnter(Environment* env, Local<Object> object) {
       }
     }
   }
-  return false;
 }
 
 
-bool DomainExit(Environment* env, v8::Local<v8::Object> object) {
+void DomainExit(Environment* env, v8::Local<v8::Object> object) {
   Local<Value> domain_v = object->Get(env->domain_string());
   if (domain_v->IsObject()) {
     Local<Object> domain = domain_v.As<Object>();
-    if (domain->Get(env->disposed_string())->IsTrue())
-      return true;
     Local<Value> exit_v = domain->Get(env->exit_string());
     if (exit_v->IsFunction()) {
       if (exit_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
@@ -1191,7 +1186,6 @@ bool DomainExit(Environment* env, v8::Local<v8::Object> object) {
       }
     }
   }
-  return false;
 }
 
 
@@ -1398,9 +1392,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
   CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
 
   if (env->using_domains()) {
-    failed_ = DomainEnter(env, object_);
+    DomainEnter(env, object_);
-    if (failed_)
-      return;
   }
 
   if (asyncContext.async_id != 0) {
@@ -1432,8 +1424,7 @@ void InternalCallbackScope::Close() {
   }
 
   if (env_->using_domains()) {
-    failed_ = DomainExit(env_, object_);
+    DomainExit(env_, object_);
-    if (failed_) return;
   }
 
   if (IsInnerMakeCallback()) {

test/parallel/test-domain-abort-when-disposed.js

@@ -1,25 +0,0 @@
-'use strict';
-
-const common = require('../common');
-const assert = require('assert');
-const domain = require('domain');
-
-// These were picked arbitrarily and are only used to trigger arync_hooks.
-const JSStream = process.binding('js_stream').JSStream;
-const Socket = require('net').Socket;
-
-const handle = new JSStream();
-handle.domain = domain.create();
-handle.domain.dispose();
-
-handle.close = () => {};
-handle.isAlive = () => { throw new Error(); };
-
-const s = new Socket({ handle });
-
-// When AsyncWrap::MakeCallback() returned an empty handle the
-// MaybeLocal::ToLocalChecked() call caused the process to abort. By returning
-// v8::Undefined() it allows the error to propagate to the 'error' event.
-s.on('error', common.mustCall((e) => {
-  assert.strictEqual(e.code, 'EINVAL');
-}));

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

@@ -1,97 +0,0 @@
-// 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.
-
-'use strict';
-
-// 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.
-
-require('../common');
-const assert = require('assert');
-const domain = require('domain');
-let 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.
-let a = 0;
-log();
-function log() {
-  console.log(a++, process.domain);
-  if (a < 10) setTimeout(log, 20);
-}
-
-let 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', 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);
-    }, 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(); // eslint-disable-line no-undef
-  });
-}, TIMEOUT_DURATION);
-
-// 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);
-
-process.on('exit', function() {
-  assert.strictEqual(a, 10);
-  assert.strictEqual(disposalFailed, false);
-  assert(secondTimerRan);
-  console.log('ok');
-});

test/parallel/test-domain-exit-dispose.js

@@ -1,62 +0,0 @@
-// 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.
-
-'use strict';
-const common = require('../common');
-const assert = require('assert');
-const domain = require('domain');
-
-// no matter what happens, we should increment a 10 times.
-let a = 0;
-log();
-function log() {
-  console.log(a++, process.domain);
-  if (a < 10) setTimeout(log, 20);
-}
-
-// in 50ms we'll throw an error.
-setTimeout(err, 50);
-function err() {
-  const d = domain.create();
-  d.on('error', handle);
-  d.run(err2);
-
-  function err2() {
-    // this timeout should never be called, since the domain gets
-    // disposed when the error happens.
-    setTimeout(common.mustNotCall(), 1);
-
-    // this function doesn't exist, and throws an error as a result.
-    err3(); // eslint-disable-line no-undef
-  }
-
-  function handle(e) {
-    // this should clean up everything properly.
-    d.dispose();
-    console.error(e);
-    console.error('in handler', process.domain, process.domain === d);
-  }
-}
-
-process.on('exit', function() {
-  assert.strictEqual(a, 10);
-  console.log('ok');
-});

test/parallel/test-domain-nested-throw.js

@@ -25,31 +25,19 @@ const assert = require('assert');
 
 const domain = require('domain');
 
-let dispose;
+if (process.argv[2] !== 'child') {
-switch (process.argv[2]) {
+  parent();
-  case 'true':
+  return;
-    dispose = true;
-    break;
-  case 'false':
-    dispose = false;
-    break;
-  default:
-    parent();
-    return;
 }
 
 function parent() {
   const node = process.execPath;
   const spawn = require('child_process').spawn;
   const opt = { stdio: 'inherit' };
-  let child = spawn(node, [__filename, 'true'], opt);
+  const child = spawn(node, [__filename, 'child'], opt);
   child.on('exit', function(c) {
     assert(!c);
-    child = spawn(node, [__filename, 'false'], opt);
+    console.log('ok');
-    child.on('exit', function(c) {
-      assert(!c);
-      console.log('ok');
-    });
   });
 }
 
@@ -77,7 +65,6 @@ function inner(throw1, throw2) {
       console.error('got domain 1 twice');
       process.exit(1);
     }
-    if (dispose) domain1.dispose();
     gotDomain1Error = true;
     throw2();
   });
@@ -108,7 +95,7 @@ process.on('exit', function() {
   assert(gotDomain2Error);
   assert(threw1);
   assert(threw2);
-  console.log('ok', dispose);
+  console.log('ok');
 });
 
 outer();

test/parallel/test-promises-unhandled-rejections.js

@@ -634,7 +634,6 @@ asyncTest(
       onUnhandledSucceed(done, function(reason, promise) {
         assert.strictEqual(reason, e);
         assert.strictEqual(domainReceivedError, domainError);
-        d.dispose();
       });
       Promise.reject(e);
       process.nextTick(function() {

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

@@ -1,46 +0,0 @@
-'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(1000);
-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;
-}