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: 4a74fc9 PR-URL: nodejs/node#15412 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
Qard · Sep 21, 2017 · 316c95e · 316c95e
1 parent a9c676c
commit 316c95e
Show file tree

Hide file tree

Showing 12 changed files with 17 additions and 336 deletions.
diff --git a/doc/api/deprecations.md b/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>

diff --git a/doc/api/domain.md b/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

diff --git a/lib/domain.js b/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
-  // domain is not on the stack.
+  // don't do anything if this 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 (this._disposed || ee.domain === this)
+  // If the domain is already added, then nothing left to do.
+  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');
diff --git a/lib/timers.js b/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();
     }
 

diff --git a/src/env.h b/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")                                                \

diff --git a/src/node.cc b/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_);
-    if (failed_)
-      return;
+    DomainEnter(env, object_);
   }
 
   if (asyncContext.async_id != 0) {
@@ -1432,8 +1424,7 @@ void InternalCallbackScope::Close() {
   }
 
   if (env_->using_domains()) {
-    failed_ = DomainExit(env_, object_);
-    if (failed_) return;
+    DomainExit(env_, object_);
   }
 
   if (IsInnerMakeCallback()) {

diff --git a/test/parallel/test-domain-abort-when-disposed.js b/test/parallel/test-domain-abort-when-disposed.js
diff --git a/test/parallel/test-domain-exit-dispose-again.js b/test/parallel/test-domain-exit-dispose-again.js