Error reporting from top level callbacks

@addaleax

What is the problem this feature will solve?

When an exception is thrown inside a top level callback, it is reported as an uncaught exception. This can only be handled by attaching an 'uncaughtException' handler on the process object.

This is inconvenient because it would require users to write a single handler for all sorts of uncaught exceptions encountered by the entire process.

What is the feature you are proposing to solve the problem?

Instead of throwing exceptions from the top level callbacks, we should emit 'error' events. This should allow users to distribute their error handling code by attaching separate handlers for each object that will emit such the event.

We’re essentially saying that Node.js code that is equivalent to

function topLevelCallback() {
  <... internal node.js code ...>
  emitter.emit('event', ...)
}

should be transformed into

function topLevelCallback() {
  try {
    <... internal node.js code ...>
  } catch (err) {
    emitter.emit('error', err);
    return;
  }
  emitter.emit('event', ...)
}

(mod being possibly/partially written in C++ rather than JS)

Originally posted by @addaleax in #42054 (comment)

This might only work for EventEmitter objects because only those are capable of emitting 'error' events. However, we should also consider turning other objects that might potentially throw exceptions from top level callbacks into EventEmitter instances but that should be discussed in a separate issue. I would like to keep this issue focused only on top level callbacks inside existing EventEmitters present in the Node.js repo.

What alternatives have you considered?

Current behavior where things are inconsistent.

cc @nodejs/tsc since this is a big change to how Node.js works.

Hmm so to understand the ask:

Everything would behave the same for users without an error event handler
Users with an error event handler would get the same behavior as today
This is only relevant for top-level methods that operate on event emitter instances - and it should only happen when handling error makes sense.

This sounds very tricky and I think a few concrete API examples would make the changes easier to grok.

is this literally asking for a codemod based on code that says <identifier>.emit()? I'm very confused.

oh is this asking for certain error handling pattern within node core itself? i thought it was talking about code-modding user code.

@benjamingr

@benjamingr

Everything would behave the same for users without an error event handler

If an 'error' event gets emitted and the EventEmitter does not already have an attached 'error' handler, it would throw an uncaught exception, so yes this would be the same as directly throwing an exception from inside the top level callback.

Users with an error event handler would get the same behavior as today

Yes, they should also be able to handle additional errors, i.e., those ones which might have been previously thrown as uncaught exceptions.

This is only relevant for top-level methods that operate on event emitter instances

That is correct.

and it should only happen when handling error makes sense.

Do you mean we should special case some errors and throw those from these top level callbacks? Users should be able to handle those anyways using process.on('uncaughtException', ...), so I think we should let users decide which all errors they would like to handle, i.e., emit all possible exceptions as 'error' events.

This sounds very tricky and I think a few concrete API examples would make the changes easier to grok.

Sure. We can consider how we are doing the error handling for the udp socket EventEmitter object in - #42054.

Details

The udp socket object is an EventEmitter instance, so that would be an appropriate example for this. I'll use this test, test/parallel/test-dgram-send-multi-string-array.js, as an example usage of the API:

What I want

With the current implementation in #42054, if AddressToJS() throws an exception, it get propagated as an uncaught exception when there is no attached 'error' event handler:

'use strict';
const common = require('../common');
const assert = require('assert');
const dgram = require('dgram');
const socket = dgram.createSocket('udp4');
const data = ['foo', 'bar', 'baz'];

socket.on('message', common.mustCall((msg, rinfo) => {
  socket.close();
  assert.deepStrictEqual(msg.toString(), data.join(''));
}));

socket.bind(() => socket.send(data, socket.address().port, 'localhost'));

$ ./node test/parallel/test-dgram-send-multi-string-array.js
node:events:505
      throw er; // Unhandled 'error' event
      ^

Error: ENXIO: no such device or address, uv_if_indextoiid
Emitted 'error' event on Socket instance at:
    at UDP.onError [as onerror] (node:dgram:928:15) {
  errno: -6,
  code: 'ENXIO',
  syscall: 'uv_if_indextoiid'
}

Node.js v18.0.0-pre
$ echo $?
1

If the user wants, they have the ability to handle the 'error' event by attaching the handler directly to the EventEmitter object:

'use strict';
const common = require('../common');
const assert = require('assert');
const dgram = require('dgram');
const socket = dgram.createSocket('udp4');
const data = ['foo', 'bar', 'baz'];

socket.on('message', common.mustCall((msg, rinfo) => {
  socket.close();
  assert.deepStrictEqual(msg.toString(), data.join(''));
}));

socket.on('error', (error) => {
  console.error('error event emitted:', error);
  socket.close();
  process.exitCode = 2;
});

socket.bind(() => socket.send(data, socket.address().port, 'localhost'));

$ ./node test/parallel/test-dgram-send-multi-string-array.js
error event emitted: [Error: ENXIO: no such device or address, uv_if_indextoiid] {
  errno: -6,
  code: 'ENXIO',
  syscall: 'uv_if_indextoiid'
}
$ echo $?
2

What I don't want

If we change #42054, to throw the exception instead of emitting the 'error' event, it would always get propagated as an uncaught exception:

'use strict';
const common = require('../common');
const assert = require('assert');
const dgram = require('dgram');
const socket = dgram.createSocket('udp4');
const data = ['foo', 'bar', 'baz'];

socket.on('message', common.mustCall((msg, rinfo) => {
  socket.close();
  assert.deepStrictEqual(msg.toString(), data.join(''));
}));

socket.bind(() => socket.send(data, socket.address().port, 'localhost'));

$ ./node test/parallel/test-dgram-send-multi-string-array.js
node:events:505
      throw er; // Unhandled 'error' event
      ^

Error: ENXIO: no such device or address, uv_if_indextoiid
Emitted 'error' event on Socket instance at:
    at UDP.onError [as onerror] (node:dgram:928:15) {
  errno: -6,
  code: 'ENXIO',
  syscall: 'uv_if_indextoiid'
}

Node.js v18.0.0-pre
$ echo $?
1

The only way, users can handle the 'error' event is by attaching an 'uncaughtException' handler to the process object:

'use strict';
const common = require('../common');
const assert = require('assert');
const dgram = require('dgram');
const socket = dgram.createSocket('udp4');
const data = ['foo', 'bar', 'baz'];

socket.on('message', common.mustCall((msg, rinfo) => {
  socket.close();
  assert.deepStrictEqual(msg.toString(), data.join(''));
}));

process.on('uncaughtException', (error) => {
  console.error('uncaughtException event emitted:', error);
  socket.close();
  process.exitCode = 2;
});

socket.bind(() => socket.send(data, socket.address().port, 'localhost'));

$ ./node test/parallel/test-dgram-send-multi-string-array.js
uncaughtException event emitted: [Error: ENXIO: no such device or address, uv_if_indextoiid] {
  errno: -6,
  code: 'ENXIO',
  syscall: 'uv_if_indextoiid'
}
$ echo $?
2

I'm not sure of other examples because, personally I haven't dealt with uncaught exceptions coming from Node.js' internal top level callbacks before. Perhaps @addaleax knows of some other examples?

@devsnek

oh is this asking for certain error handling pattern within node core itself? i thought it was talking about code-modding user code.

@devsnek nope, not user code. Just Node.js' internal top level callbacks.

To clarify on

Users with an error event handler would get the same behavior as today
Yes, they should also be able to handle additional errors, i.e., those ones which might have been previously thrown as uncaught exceptions.

Even though they have an error even handler, the error emitted would be different so that the existing handler would not handle it so an uncaught exception would still occur, right?

I'm guessing that performance should not be a big deal because this is the error path, right?

I think this is handled by 'captureRejections' https://nodejs.org/api/events.html#capture-rejections-of-promises in the most performant way. We could start flipping the config for a few critical objects.

@mhdawson

@mhdawson

To clarify on

Users with an error event handler would get the same behavior as today
Yes, they should also be able to handle additional errors, i.e., those ones which might have been previously thrown as uncaught exceptions.

Even though they have an error even handler, the error emitted would be different so that the existing handler would not handle it so an uncaught exception would still occur, right?

If there is an existing 'error' event handler and the newly emitted error event is not handled by the user, it would get swallowed silently and no uncaught exception would be thrown.

Details

test.js:

const { EventEmitter } = require('events');

const ee = new EventEmitter();

ee.on('error', (err) => {
  // ENOENT would get silently swallowed.
  if (err === 'EPERM') {
    console.error('\'error\' event emitted:', err);
    process.exitCode = 1;
  }
});

setTimeout(() => {
  ee.emit('error', 'EPERM');
}, 500);

// Newly emitted error event.
setTimeout(() => {
  ee.emit('error', 'ENOENT');
}, 1000);

output:

$ time node test.js
'error' event emitted: EPERM
node test.js  0.05s user 0.01s system 6% cpu 1.056 total
$ echo $?
1

I'm guessing that performance should not be a big deal because this is the error path, right?

✅

@mcollina

@mcollina

I think this is handled by 'captureRejections' https://nodejs.org/api/events.html#capture-rejections-of-promises in the most performant way. We could start flipping the config for a few critical objects.

IIUC, captureRejections solves the problem only for async callbacks but this issue is also talking about handling exceptions thrown from synchronous callbacks.

I'm not sure of other examples because, personally I haven't dealt with uncaught exceptions coming from Node.js' internal top level callbacks before.

In that case, it may not be such a big change.

Then does #42054 look good to y'all?

Navigation Menu

Repository navigation

Error reporting from top level callbacks #42412

What is the problem this feature will solve?

What is the feature you are proposing to solve the problem?

What alternatives have you considered?

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Error reporting from top level callbacks #42412

Description

What is the problem this feature will solve?

What is the feature you are proposing to solve the problem?

What alternatives have you considered?

Activity

RaisinTen commented on Mar 20, 2022

benjamingr commented on Mar 20, 2022

devsnek commented on Mar 20, 2022

devsnek commented on Mar 20, 2022

RaisinTen commented on Mar 21, 2022

RaisinTen commented on Mar 21, 2022

mhdawson commented on Mar 21, 2022

mcollina commented on Mar 21, 2022

RaisinTen commented on Mar 22, 2022

RaisinTen commented on Mar 22, 2022

targos commented on Mar 22, 2022

RaisinTen commented on Mar 25, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions