From 77da4107920d585e13c1a227a60e5fcde0acccf6 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Thu, 14 Sep 2017 17:58:53 +0200
Subject: [PATCH] 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
---
doc/api/deprecations.md | 4 +-
doc/api/domain.md | 18 +---
lib/domain.js | 48 +--------
lib/timers.js | 9 --
src/env.h | 1 -
src/node.cc | 17 +---
.../test-domain-abort-when-disposed.js | 25 -----
.../test-domain-exit-dispose-again.js | 97 -------------------
test/parallel/test-domain-exit-dispose.js | 62 ------------
test/parallel/test-domain-nested-throw.js | 25 ++---
.../test-promises-unhandled-rejections.js | 1 -
...timers-unref-active-unenrolled-disposed.js | 46 ---------
12 files changed, 17 insertions(+), 336 deletions(-)
delete mode 100644 test/parallel/test-domain-abort-when-disposed.js
delete mode 100644 test/parallel/test-domain-exit-dispose-again.js
delete mode 100644 test/parallel/test-domain-exit-dispose.js
delete mode 100644 test/parallel/test-timers-unref-active-unenrolled-disposed.js
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index 5d7b3b5ff16357..d1175d480bc230 100644
--- 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
index a4a31d4fecd1f2..285378c3bd56b5 100644
--- 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
index 5cef123da82b54..1006e2a0f501ce 100644
--- 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
index 5f3982fa4a9620..28c224dbba3610 100644
--- 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
index 315d4fdc1852a4..347142a1b71fb4 100644
--- 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
index 8999014713d21a..9dd269dde3800a 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1162,12 +1162,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()) {
@@ -1176,16 +1174,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()) {
@@ -1194,7 +1189,6 @@ bool DomainExit(Environment* env, v8::Local<v8::Object> object) {
}
}
}
- return false;
}
@@ -1401,9 +1395,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) {
@@ -1435,8 +1427,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
deleted file mode 100644
index 3a02b1e94c1b11..00000000000000
--- a/test/parallel/test-domain-abort-when-disposed.js
+++ /dev/null
@@ -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');
-}));
diff --git a/test/parallel/test-domain-exit-dispose-again.js b/test/parallel/test-domain-exit-dispose-again.js
deleted file mode 100644
index 31a1ff598a32a8..00000000000000
--- a/test/parallel/test-domain-exit-dispose-again.js
+++ /dev/null
@@ -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');
-});
diff --git a/test/parallel/test-domain-exit-dispose.js b/test/parallel/test-domain-exit-dispose.js
deleted file mode 100644
index 0bc8adae828437..00000000000000
--- a/test/parallel/test-domain-exit-dispose.js
+++ /dev/null
@@ -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');
-});
diff --git a/test/parallel/test-domain-nested-throw.js b/test/parallel/test-domain-nested-throw.js
index f7fcbcad648b15..ec016ada72858d 100644
--- a/test/parallel/test-domain-nested-throw.js
+++ b/test/parallel/test-domain-nested-throw.js
@@ -25,31 +25,19 @@ const assert = require('assert');
const domain = require('domain');
-let dispose;
-switch (process.argv[2]) {
- case 'true':
- dispose = true;
- break;
- case 'false':
- dispose = false;
- break;
- default:
- parent();
- return;
+if (process.argv[2] !== 'child') {
+ 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);
- child.on('exit', function(c) {
- assert(!c);
- console.log('ok');
- });
+ 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();
diff --git a/test/parallel/test-promises-unhandled-rejections.js b/test/parallel/test-promises-unhandled-rejections.js
index 7a367920b0fd49..4494e5084d224c 100644
--- a/test/parallel/test-promises-unhandled-rejections.js
+++ b/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() {
diff --git a/test/parallel/test-timers-unref-active-unenrolled-disposed.js b/test/parallel/test-timers-unref-active-unenrolled-disposed.js
deleted file mode 100644
index 675da017173e88..00000000000000
--- a/test/parallel/test-timers-unref-active-unenrolled-disposed.js
+++ /dev/null
@@ -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;
-}