Node: Add 'listenererror' in all entities

[ibc]

• 'listenererror' event is emitted when an event listener throws (only if it's a synchronous error, otherwise an unhandled rejection will happen anyway).
• Usage:

  consumer.on('listenererror', (eventName, error) => { ... });

• Fixes issue TS: expect() statements inside event handlers are not taking any effect  #1188.

Bonus Tracks:

• Remove test.only in test-Producer.ts and fix some tests in that file.

[ibc]

To be clear: in this PR, if anything throws within any dataConsumer or dataProducer event listener in test-DirectTransport.ts, the test should fail. But it doesn't (see CI) :(

[jmillan]

👍

[piranna]

So, if an event listener throws an error, a new listenerror is emitted? Why not use error event? It's special one because if it's not handled, it's thrown as an unhandled exception, so it doesn't get unnoticed.

[ibc]

So, if an event listener throws an error, a new listenerror is emitted? Why not use error event? It's special one because if it's not handled, it's thrown as an unhandled exception, so it doesn't get unnoticed.

So you literally mean emitting "error" instead of "listenererror" within safeEmit() right?

It makes sense. Only problem is that we loose the "eventName" argument but we may emit a custom Error class with that field exposed by a public getter.

[piranna]

So you literally mean emitting "error" instead of "listenererror" within safeEmit() right?

What I mean is, from the Node.js docs:

When an error occurs within an EventEmitter instance, the typical action is for an 'error' event to be emitted. These are treated as special cases within Node.js.

If an EventEmitter does not have at least one listener registered for the 'error' event, and an 'error' event is emitted, the error is thrown, a stack trace is printed, and the Node.js process exits.

[ibc]

That's the classic controversial documentation that doesn't honor reality:

Emitting "error" event without an "error" event listener should exit the process (yes, it does):

import { EventEmitter } from 'node:events';
import * as http from 'node:http';

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

server.listen(5005, '0.0.0.0', () => console.log('server listening'));

const emitter = new EventEmitter();

emitter.on('foo', () =>
{
	console.log(
		'"foo" event, emitting unhandled "error" event that should terminate the process (but it does not)'
	);

	emitter.emit('error', new TypeError('LALALA'));
});

setTimeout(() =>
{
	console.log('timer expired, emitting "foo"');

	emitter.emit("foo");

	console.log('"foo" emitted');
}, 2000);

Output:

server listening
timer expired, emitting "foo"
"foo" event, emitting unhandled "error" event that should terminate the process (but it does not)
node:events:492
      throw er; // Unhandled 'error' event
      ^

TypeError: LALALA
    at EventEmitter.<anonymous> (file:///private/tmp/kk/bar2.mjs:25:24)
    at EventEmitter.emit (node:events:514:28)
    at Timeout._onTimeout (file:///private/tmp/kk/bar2.mjs:32:10)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)
Emitted 'error' event at:
    at EventEmitter.<anonymous> (file:///private/tmp/kk/bar2.mjs:25:10)
    at EventEmitter.emit (node:events:514:28)
    [... lines matching original stack trace ...]
    at process.processTimers (node:internal/timers:514:7)

Throwing an error within an event listener should generate "error" event (but it doesn't):

import { EventEmitter } from 'node:events';
import * as http from 'node:http';

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

server.listen(5005, '0.0.0.0', () => console.log('server listening'));

const emitter = new EventEmitter();

emitter.on('error', (error) =>
{
	console.log('"error" event: %s', String(error));
});

emitter.on('foo', () =>
{
	console.log(
		'"foo" event, throwing an error that should produce an "error" event (but it does not)'
	);

	throw new Error('LALALA');
});

setTimeout(() =>
{
	console.log('timer expired, emitting "foo"');

	emitter.emit("foo");

	console.log('"foo" emitted');
}, 2000);

Output:

server listening
timer expired, emitting "foo"
"foo" event, throwing an error that should produce an "error" event (but it does not)
file:///private/tmp/kk/bar2.mjs:25
	throw new Error('LALALA');
	^

Error: LALALA
    at EventEmitter.<anonymous> (file:///private/tmp/kk/bar2.mjs:25:8)
    at EventEmitter.emit (node:events:514:28)
    at Timeout._onTimeout (file:///private/tmp/kk/bar2.mjs:32:10)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)

[ibc]

If we make our safeEmit() method emit "error" when the user given listener throws, then we would terminate the entire process unless the user creates an "error" listener (in every Transport, Producer, etc). That would be terrible from backwards compatibility. We cannot do that at this point.

[piranna]

Throwing an error within an event listener should generate "error" event (but it doesn't):

I don't think it should emit an error event, they must be explicitly emitted, throwing it as an unhandled error is the correct way here.

If we make our safeEmit() method emit "error" when the user given listener throws, then we would terminate the entire process unless the user creates an "error" listener (in every Transport, Producer, etc). That would be terrible from backwards compatibility. We cannot do that at this point.

Agree, fair enough. We can consider it for the 4.0 version, specially regarding all places where logger.error() is being call when in fact errors should be thrown instead, maybe as Promises for API homogeneity.

[ibc]

Agree, fair enough. We can consider it for the 4.0 version, specially regarding all places where logger.error() is being call when in fact errors should be thrown instead, maybe as Promises for API homogeneity.

Yes.