Skip to content

domain: allow concurrent user-land impl #26326

New issue

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

#

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

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from
Closed

domain: allow concurrent user-land impl #26326

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
17 changes: 0 additions & 17 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,23 +818,6 @@ A signing `key` was not provided to the [`sign.sign()`][] method.

`c-ares` failed to set the DNS server.

<a id="ERR_DOMAIN_CALLBACK_NOT_AVAILABLE"></a>
### ERR_DOMAIN_CALLBACK_NOT_AVAILABLE

The `domain` module was not usable since it could not establish the required
error handling hooks, because
[`process.setUncaughtExceptionCaptureCallback()`][] had been called at an
earlier point in time.

<a id="ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE"></a>
### ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE

[`process.setUncaughtExceptionCaptureCallback()`][] could not be called
because the `domain` module has been loaded at an earlier point in time.

The stack trace is extended to include the point in time at which the
`domain` module had been loaded.

Copy link
Member

Choose a reason for hiding this comment

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

We have a special section for error codes that were removed, could you move these there (and add a removed: REPLACEME YAML tag)?

Copy link
Author

Choose a reason for hiding this comment

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

Yes 👍 I keep forgetting about that, my apologies.

<a id="ERR_ENCODING_INVALID_ENCODED_DATA"></a>
### ERR_ENCODING_INVALID_ENCODED_DATA

Expand Down
9 changes: 2 additions & 7 deletions doc/api/repl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,8 @@ global or scoped variable, the input `fs` will be evaluated on-demand as
The REPL uses the [`domain`][] module to catch all uncaught exceptions for that
REPL session.

This use of the [`domain`][] module in the REPL has these side effects:

* Uncaught exceptions do not emit the [`'uncaughtException'`][] event.
* Trying to use [`process.setUncaughtExceptionCaptureCallback()`][] throws
an [`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`][] error.
This use of the [`domain`][] module in the REPL has the side effect of making
uncaught exceptions not emit the [`'uncaughtException'`][] event.

#### Assignment of the `_` (underscore) variable
<!-- YAML
Expand Down Expand Up @@ -636,9 +633,7 @@ For an example of running a REPL instance over [curl(1)][], see:

[`'uncaughtException'`]: process.html#process_event_uncaughtexception
[`--experimental-repl-await`]: cli.html#cli_experimental_repl_await
[`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`]: errors.html#errors_err_domain_cannot_set_uncaught_exception_capture
[`domain`]: domain.html
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`readline.InterfaceCompleter`]: readline.html#readline_use_of_the_completer_function
[`readline.Interface`]: readline.html#readline_class_interface
[`repl.ReplServer`]: #repl_class_replserver
Expand Down
87 changes: 12 additions & 75 deletions lib/domain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,17 @@

const util = require('util');
const EventEmitter = require('events');
const {
ERR_DOMAIN_CALLBACK_NOT_AVAILABLE,
ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE,
ERR_UNHANDLED_ERROR
} = require('internal/errors').codes;
const { createHook } = require('async_hooks');

// TODO(addaleax): Use a non-internal solution for this.
const kWeak = Symbol('kWeak');
const { WeakReference } = internalBinding('util');

// Communicate with events module, but don't require that
// module to have to load this one, since this module has
// a few side effects.
EventEmitter.usingDomains = true;

// Overwrite process.domain with a getter/setter that will allow for more
// effective optimizations
var _domain = [null];
Expand Down Expand Up @@ -80,23 +80,7 @@ const asyncHook = createHook({
}
});

// When domains are in use, they claim full ownership of the
// uncaught exception capture callback.
if (process.hasUncaughtExceptionCaptureCallback()) {
throw new ERR_DOMAIN_CALLBACK_NOT_AVAILABLE();
}

// Get the stack trace at the point where `domain` was required.
// eslint-disable-next-line no-restricted-syntax
const domainRequireStack = new Error('require(`domain`) at this point').stack;

const { setUncaughtExceptionCaptureCallback } = process;
process.setUncaughtExceptionCaptureCallback = function(fn) {
const err = new ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE();
err.stack = err.stack + '\n' + '-'.repeat(40) + '\n' + domainRequireStack;
throw err;
};


let sendMakeCallbackDeprecation = false;
function emitMakeCallbackDeprecation() {
Expand All @@ -123,10 +107,10 @@ function topLevelDomainCallback(cb, ...args) {
return ret;
}

// It's possible to enter one domain while already inside
// another one. The stack is each entered domain.
const stack = [];
exports._stack = stack;

// It's possible to enter one domain while already inside another one. The stack
// is each entered domain.
const stack = exports._stack = process._domainsStack;
Copy link
Member

Choose a reason for hiding this comment

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

can you revert the change to this line?

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean? How would stack and exports._stack be initialized?

Copy link
Member

Choose a reason for hiding this comment

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

The general goal is to avoid doing x = y = z

this would work:

const stack = process._domainStack;
exports._stack = stack; // no change to this line

or better yet, remove the const stack = entirely and just refer to process._domainStack.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, will do!

internalBinding('domain').enable(topLevelDomainCallback);

function updateExceptionCapture() {
Expand Down Expand Up @@ -181,6 +165,8 @@ class Domain extends EventEmitter {
}
}

Domain[EventEmitter.domainSym] = true;

exports.Domain = Domain;

exports.create = exports.createDomain = function createDomain() {
Expand Down Expand Up @@ -307,7 +293,7 @@ Domain.prototype.add = function(ee) {
// d.add(e);
// e.add(d);
// e.emit('error', er); // RangeError, stack overflow!
if (this.domain && (ee instanceof Domain)) {
if (this.domain && ee[EventEmitter.domainSym]) {
for (var d = this.domain; d; d = d.domain) {
if (ee === d) return;
}
Expand Down Expand Up @@ -410,52 +396,3 @@ Domain.prototype.bind = function(cb) {

return runBound;
};

// Override EventEmitter methods to make it domain-aware.
EventEmitter.usingDomains = true;

const eventInit = EventEmitter.init;
EventEmitter.init = function() {
this.domain = null;
if (exports.active && !(this instanceof exports.Domain)) {
this.domain = exports.active;
}

return eventInit.call(this);
};

const eventEmit = EventEmitter.prototype.emit;
EventEmitter.prototype.emit = function(...args) {
const domain = this.domain;

const type = args[0];
const shouldEmitError = type === 'error' &&
this.listenerCount(type) > 0;

// Just call original `emit` if current EE instance has `error`
// handler, there's no active domain or this is process
if (shouldEmitError || domain === null || domain === undefined ||
this === process) {
return Reflect.apply(eventEmit, this, args);
}

if (type === 'error') {
const er = args.length > 1 && args[1] ?
args[1] : new ERR_UNHANDLED_ERROR();

if (typeof er === 'object') {
er.domainEmitter = this;
er.domain = domain;
er.domainThrown = false;
}

domain.emit('error', er);
return false;
}

domain.enter();
const ret = Reflect.apply(eventEmit, this, args);
domain.exit();

return ret;
};
58 changes: 45 additions & 13 deletions lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ module.exports = EventEmitter;
EventEmitter.EventEmitter = EventEmitter;

EventEmitter.usingDomains = false;
EventEmitter.domainSym = Symbol('isDomainLike');
Copy link
Member

@devsnek devsnek Feb 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

symbol descriptions should match how they are accessed. Symbol('events.domainSym')

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, will fix!

Copy link
Member

Choose a reason for hiding this comment

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

I’d prefer EventEmitter.isDomainLike in that case :) Also, should this be documented, since the idea is making it available to other implementations (it is, right)?

Copy link
Author

Choose a reason for hiding this comment

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

I’d prefer EventEmitter.isDomainLike in that case :)

Sounds good, given there might be a lot of different opinions on the naming of this property, let's leave that to the final stages of this review :)

Also, should this be documented, since the idea is making it available to other implementations (it is, right)?

Yes, the idea is to make it available to other implementations. You can take a look at how it's used in the proof-of-concept that I wrote to test this.


EventEmitter.prototype.domain = undefined;
EventEmitter.prototype._events = undefined;
EventEmitter.prototype._eventsCount = 0;
EventEmitter.prototype._maxListeners = undefined;
Expand Down Expand Up @@ -72,6 +74,12 @@ Object.defineProperty(EventEmitter, 'defaultMaxListeners', {
});

EventEmitter.init = function() {
this.domain = null;
// If there is an active domain, then attach to it, except if the event
// emitter being constructed is itself an instance of a domain-like class.
if (EventEmitter.usingDomains && !this.constructor[EventEmitter.domainSym]) {
this.domain = process.domain;
}

if (this._events === undefined ||
this._events === Object.getPrototypeOf(this)._events) {
Expand Down Expand Up @@ -152,12 +160,25 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
else if (!doError)
return false;

const domain = this.domain;

// If there is no 'error' event listener then throw.
if (doError) {
let er;
if (args.length > 0)
er = args[0];
if (er instanceof Error) {
if (domain !== null && domain !== undefined) {
if (!er) {
const errors = lazyErrors();
er = new errors.ERR_UNHANDLED_ERROR();
}
if (typeof er === 'object' && er !== null) {
er.domainEmitter = this;
er.domain = domain;
er.domainThrown = false;
}
domain.emit('error', er);
} else if (er instanceof Error) {
try {
const { kExpandStackSymbol } = require('internal/util');
const capture = {};
Expand All @@ -171,28 +192,36 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
// Note: The comments on the `throw` lines are intentional, they show
// up in Node's output if this results in an unhandled exception.
throw er; // Unhandled 'error' event
}
} else {
let stringifiedEr;
const { inspect } = require('internal/util/inspect');
try {
stringifiedEr = inspect(er);
} catch {
stringifiedEr = er;
}

let stringifiedEr;
const { inspect } = require('internal/util/inspect');
try {
stringifiedEr = inspect(er);
} catch {
stringifiedEr = er;
// At least give some kind of context to the user
const errors = lazyErrors();
const err = new errors.ERR_UNHANDLED_ERROR(stringifiedEr);
err.context = er;
throw err; // Unhandled 'error' event
}

// At least give some kind of context to the user
const errors = lazyErrors();
const err = new errors.ERR_UNHANDLED_ERROR(stringifiedEr);
err.context = er;
throw err; // Unhandled 'error' event
return false;
}

const handler = events[type];

if (handler === undefined)
return false;

let needDomainExit = false;
if (domain !== null && domain !== undefined && this !== process) {
domain.enter();
needDomainExit = true;
}

if (typeof handler === 'function') {
Reflect.apply(handler, this, args);
} else {
Expand All @@ -202,6 +231,9 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
Reflect.apply(listeners[i], this, args);
}

if (needDomainExit)
domain.exit();

return true;
};

Expand Down
9 changes: 0 additions & 9 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,15 +599,6 @@ E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH',
'Input buffers must have the same length', RangeError);
E('ERR_DNS_SET_SERVERS_FAILED', 'c-ares failed to set servers: "%s" [%s]',
Error);
E('ERR_DOMAIN_CALLBACK_NOT_AVAILABLE',
'A callback was registered through ' +
'process.setUncaughtExceptionCaptureCallback(), which is mutually ' +
'exclusive with using the `domain` module',
Error);
E('ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE',
'The `domain` module is in use, which is mutually exclusive with calling ' +
'process.setUncaughtExceptionCaptureCallback()',
Error);
E('ERR_ENCODING_INVALID_ENCODED_DATA',
'The encoded data was not valid for encoding %s', TypeError);
E('ERR_ENCODING_NOT_SUPPORTED', 'The "%s" encoding is not supported',
Expand Down
10 changes: 0 additions & 10 deletions test/parallel/test-domain-load-after-set-uncaught-exception-capture.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,4 @@ const common = require('../common');

process.setUncaughtExceptionCaptureCallback(common.mustNotCall());

common.expectsError(
() => require('domain'),
{
code: 'ERR_DOMAIN_CALLBACK_NOT_AVAILABLE',
type: Error,
message: /^A callback was registered.*with using the `domain` module/
}
);

process.setUncaughtExceptionCaptureCallback(null);
require('domain'); // Should not throw.
28 changes: 0 additions & 28 deletions test/parallel/test-domain-set-uncaught-exception-capture-after-load.js
View file
Open in desktop

This file was deleted.