Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

domain: remove .dispose() #15412

Closed
wants to merge 1 commit into from
Closed

domain: remove .dispose() #15412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions doc/api/deprecations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea to expand on the description in here to explain why it was removed and why it is an anti-pattern. That doesn't have to be in this PR, but it would be worthwhile at some point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we had a whole document explaining why and how domains were broken, including .dispose()? I can’t seem to find it anywhere in our repos, though…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh.. there's something floating around somewhere but I cannot remember where.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The domains postmortem is at https://nodejs.org/en/docs/guides/domain-postmortem/


<a id="DEP0013"></a>
Expand Down
18 changes: 1 addition & 17 deletions doc/api/domain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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`
Expand All @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
48 changes: 4 additions & 44 deletions lib/domain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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.
Expand Down Expand Up @@ -224,9 +212,6 @@ Domain.prototype.remove = function(ee) {


Domain.prototype.run = function(fn) {
if (this._disposed)
return;

var ret;

this.enter();
Expand All @@ -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, {
Expand Down Expand Up @@ -291,9 +273,6 @@ Domain.prototype.intercept = function(cb) {


function bound(_this, self, cb, fnargs) {
if (self._disposed)
return;

var ret;

self.enter();
Expand All @@ -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');
9 changes: 0 additions & 9 deletions lib/timers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah. so disposal has never worked for setImmediate(). let's use that as a demonstration of how little it must have been used.


domain.enter();
}

Expand Down
1 change: 0 additions & 1 deletion src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand Down
17 changes: 4 additions & 13 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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()) {
Expand All @@ -1194,7 +1189,6 @@ bool DomainExit(Environment* env, v8::Local<v8::Object> object) {
}
}
}
return false;
}


Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -1435,8 +1427,7 @@ void InternalCallbackScope::Close() {
}

if (env_->using_domains()) {
failed_ = DomainExit(env_, object_);
if (failed_) return;
DomainExit(env_, object_);
}

if (IsInnerMakeCallback()) {
Expand Down
25 changes: 0 additions & 25 deletions test/parallel/test-domain-abort-when-disposed.js
View file
Open in desktop

This file was deleted.

97 changes: 0 additions & 97 deletions test/parallel/test-domain-exit-dispose-again.js
View file
Open in desktop

This file was deleted.

Loading