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.

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 error handler executed in context of errored domain. #26086

Closed
cprussin opened this issue Feb 13, 2019 · 1 comment
Closed

Domain error handler executed in context of errored domain. #26086

cprussin opened this issue Feb 13, 2019 · 1 comment
Labels
domain Issues and PRs related to the domain subsystem.

Comments

@cprussin
Copy link

cprussin commented Feb 13, 2019

  • Version: v10.15.1 (almost definitely present in older versions as well though)
  • Platform: Linux lyra 4.14.97 #1-NixOS SMP Thu Jan 31 07:13:48 UTC 2019 x86_64 GNU/Linux
  • Subsystem: Domains

Currently, domains execute the error event within the context of the domain that caught the error. I'm not sure if this is entirely intentional; however, this can easily lead to an infinite loop when using a stream within the error handler due to the domains stack growing at the same rate it shrinks, e.g.:

const domain = require('domain');
const http = require('http');

const aDomain = domain.create();
aDomain.on('error', () => {
    console.log("Stack length: " + domain._stack.length);
    http.get('http://google.com', () => { throw new Error('foobar'); });
});
aDomain.run(() => { throw new Error('fail') });

A workaround is to use aDomain.once instead of aDomain.on to watch the error event, but this workaround will only work for a certain class of consumers of domains--that is, cases where the domain is no longer used after the first error occurs.

It's also possible to manually create a new domain and execute the error event handler within that new domain to avoid this issue.

I believe that domain error handlers should not be executed within the same domain that they're handling. However, I'm not sure if that would be considered a breaking change or a bugfix.

Also I'm not sure if there are any other ways to solve this issue and avoid the infinite loop that may not be considered breaking changes?

@misterdjules misterdjules added the domain Issues and PRs related to the domain subsystem. label Feb 14, 2019
misterdjules pushed a commit to misterdjules/node-1 that referenced this issue Feb 20, 2019
Before this change, domains' error handlers would run with the
corresponding domain as the active domain. This creates the
possibility for domains' error handlers to call themselves recursively
if an event emitter created in the error handler emits an error, or if
the error handler throws an error.

This change sets the active domain to be the domain's parent (or null
if the domain for which the error handler is called has no parent) to
prevent that from happening.

Fixes: nodejs#26086
@misterdjules
Copy link

FWIW, candidate fix open for discussion at #26211.

@Trott Trott closed this as completed in 43a5170 Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain Issues and PRs related to the domain subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants