nodejs / nodejs/node

error handler of same domain can be called several times when it throws

Open
#25,505 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

domain help wanted
Dominant language
JavaScript
Stars
122k
Forks
37.3k
Avg merge
4d 2h
Merged PRs (30d)
283

Description

  • Version: Current tip of master (66f45e7e5b), but probably applies to all versions.
  • Platform: All platforms.
  • Subsystem: domain.

The following code:

'use strict';

const common = require('../common');
const domain = require('domain');
const http = require('http');

const server = http.createServer((req, res) => {
  res.end();
});

let numDomainErrorListenerCalls = 0;

function performHttpRequestWithDomain(cb) {
  const d = domain.create();
  d.run(() => {
    const req = http.get({
      host: '127.0.0.1', port: server.address().port
    }, (res) => {
      res.on('data', () => {});
      res.on('end', () => {
        throw new Error('bang');
      });
    });

    req.end();
  });

  d.on('error', (domainErr) => {
    console.log(++numDomainErrorListenerCalls);
    throw new Error('boom');
  });
}

server.listen(0, '127.0.0.1', () => {
  performHttpRequestWithDomain(common.mustCall(() => {
    server.close();
  }));
});

gives the following output:

$ ./node test/parallel/test-http-req-domain-stack.js 
1
2
/Users/jgilli/dev/node/test/parallel/test-http-req-domain-stack.js:30
    throw new Error('boom');
    ^

Error: boom
    at Domain.d.on (/Users/jgilli/dev/node/test/parallel/test-http-req-domain-stack.js:30:11)
    at Domain.emit (events.js:188:13)
    at Domain.EventEmitter.emit (domain.js:430:20)
    at Domain._errorHandler (domain.js:216:23)
    at Domain._errorHandler (domain.js:244:33)
    at Object.setUncaughtExceptionCaptureCallback (domain.js:132:29)
    at process._fatalException (internal/process/execution.js:102:29)

The uncaught exception is expected. What is not expected as far as I understand is for the same domain's error handler to run more than once.

I believe the original intention of the domain's implementation is to pop the domains stack when a domain's error handler throw, so that the domain that handles that new error is the "parent" domain.

However, in the example above the same domain is pushed on the stack more than once. For instance, when an event is emitted from a nextTick callback, the same domain will be entered from the nextTick callbacks scheduler and then once again from the event emitter.

Pushing the same domain on the stack more than once makes sense so that the components that push a domain can pop it from the stack. However, I think we could probably replace the call to pop the stack once in the domain error handling code to remove all consecutive instances of that domain instead.

@nodejs/domains Thoughts?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with test/parallel/test-http-req-domain-stack.js and reproduce the repeated error-listener calls. Read lib/domain.js alongside internal/process/next_tick.js, especially the referenced stack-handling paths. Done means the regression is covered and the same domain's error handler is no longer called repeatedly when it throws.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, nodejs
Domain
backend, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.