Make `emit()` wait for all listeners to settle #19

dinoboff · 2017-12-17T13:16:24Z

Currently, emittery#emit does settle early if one of the listener rejects. Either its documentation should be changed (if it's exclusively used to handle error, the behaviour is fine) or it first iterate over each listener result an wait for it to settle and reject with the first error at the end of the iteration.

WIP: only implemented the test.

novemberborn · 2017-12-29T16:26:36Z

This is quite an interesting tradeoff! I prefer the current behavior, since it's more analogous to Promise.all() which also rejects immediately. This does cause subsequent errors to be lost, of course. If we're making any change at all I'd favor collecting all errors and rejecting with a new "collection" error.

novemberborn · 2018-01-05T13:48:46Z

@sindresorhus what do you think?

dinoboff · 2018-01-05T15:35:22Z

The current implementation support Node's EventEmitter default error handler with ee.emit('foo').catch(e => {console.error(e); process.exist(1);}).

It doesn't support handling errors like Node's EventEmitter's "error" event but I can't think of a use case that would require the producer to handle every errors with emit; I don't think collecting all errors and resolving emit with all of them would be useful.

Waiting for all listener responses to settle could be useful for concurrent event handling, e.g. to restrict how many events can be processed concurrently.

novemberborn · 2018-01-06T13:51:14Z

I think you're right that from the producer's perspective it's more important to know when all listeners completed than whether a particular listener erred. @sindresorhus what do you think?

novemberborn · 2018-01-08T16:03:00Z

@dinoboff I've implemented this now, though with #27 merged in. Please let me know what you think.

sindresorhus · 2018-01-15T00:46:07Z

@dinoboff Why did you close?

dinoboff · 2018-01-15T16:04:32Z

My mistake, I though it was fix.

ps: I though @novemberborn commits were in an other PR.

dinoboff · 2018-01-15T16:17:14Z

Supporting something like ee.emit('foo').catch(e => { process.exist(1); }) would become problematic if one listener is slow or never settle.

My feeling is having an error event like EventEmitter would be better to handle error, although without the default error handler stopping the process.

novemberborn · 2018-01-15T16:47:39Z

Similarly, I've been wondering about what happens when multiple listeners throw. It still seems wrong to just lose those failures. Thus the only way to safely write listeners is to put try-catch statements inside them…

Perhaps emit() should never reject? If it doesn't .catch() the listener promises then those will become unhandled rejections which are already available through process.on('unhandledRejection') and will likely cause future Node.js versions to exit.

novemberborn · 2018-01-20T16:18:43Z

@dinoboff @sindresorhus how do you think emit() should behave? Reject with the first error, reject with all errors, or never reject? Should emitSerial() follow suite?

novemberborn · 2018-01-21T15:21:43Z

That is to say, I'm not convinced listeners should be able to impact whether await this.emit('event') throws. It makes it harder to write predictable APIs if they can suddenly throw third-party errors.

I think if we allocate a new Promise.rejected(err) value that we don't return, then we can let Node.js' "unhandled rejection" infrastructure handle these cases. Alternatively Emittery could have a separate event that should be subscribed to so that errors can be logged etc, but application developers probably won't know to utilize that anyway.

sindresorhus · 2018-02-17T19:21:18Z

That is to say, I'm not convinced listeners should be able to impact whether await this.emit('event') throws. It makes it harder to write predictable APIs if they can suddenly throw third-party errors.

I agree. I don't think listener should be able to make emit throw.

I think if we allocate a new Promise.rejected(err) value that we don't return, then we can let Node.js' "unhandled rejection" infrastructure handle these cases. Alternatively Emittery could have a separate event that should be subscribed to so that errors can be logged etc, but application developers probably won't know to utilize that anyway.

How about both? If the user doesn't subscribe to the error event, it becomes a "unhandled rejection". That way we're never silent on failure, but also let devs handle event errors separately if needed.

* The promise returned by emit() settles once all listeners do * If no listener threw an exception, the promise is fulfilled with `undefined` * Otherwise it's rejected with the first exception

dinoboff · 2018-02-18T02:27:55Z

@sindresorhus Something like that?
@novemberborn I don't know how I broke that test.

novemberborn · 2018-02-18T14:26:14Z

@sindresorhus If we do have a separate event I think we should export a dedicated instance to which those errors are routed. Normal instances shouldn't emit events when consuming code fails. We'd have to document that this should not be used by library code, only by applications.

novemberborn · 2018-02-18T14:33:32Z

@dinoboff I can't immediately see why the behavior is different, but I don't think emitting error should result in an unhandled rejection. Emittery is trying to get away from that behavior. I think what @sindresorhus was arguing is that listener errors should be re-emitted as errors, as long as there's listeners for that event. If not they should be unhandled rejections.

I don't think listeners should (indirectly) influence what events are emitted. That implies separate infrastructure for application developers to be able to capture these errors. But that's tricky too since they may not be aware of Emittery or there may be multiple copies of emittery loaded into memory. I suppose we could emit (with a registered symbol) on process but that couples us to Node.js. IMHO the best approach is to leave these rejections unhandled, without influencing the promise returned by emit().

dinoboff · 2018-02-19T16:04:56Z

@novemberborn How can we leave them unhandled while catching them to make sure emitresolves once all listeners settles (and doesn't reject itself)?

Do you mean we should just swallow them?

dinoboff · 2018-02-19T16:09:32Z

The uncovered part is the one letting node log the handled rejections. I don't know how to test them since ava with fail if there are any handled rejection.

novemberborn · 2018-02-19T17:15:54Z

How can we leave them unhandled while catching them to make sure emitresolves once all listeners settles (and doesn't reject itself)?

Do you mean we should just swallow them?

I think we'd need to allocate a new Promise.reject(err) and not return that. So yes we'd swallow the error.

The uncovered part is the one letting node log the handled rejections. I don't know how to test them since ava with fail if there are any handled rejection.

You could add an unhandledRejection handler. It receives the promise. If you then .catch() that promise AVA should regard that promise as now being handled. See https://nodejs.org/api/process.html#process_event_unhandledrejection and https://github.com/jamestalmage/currently-unhandled.

dinoboff · 2018-02-20T23:23:35Z

@novemberborn

I think we'd need to allocate a new Promise.reject(err) and not return that. So yes we'd swallow the error.

That's what the PR hostReportError(err) does and by "swallowing" the error I meant removing node unhandled rejection log (or not emitting an error event).

You could add an unhandledRejection handler. It receives the promise. If you then .catch() that promise AVA should regard that promise as now being handled. See https://nodejs.org/api/process.html#process_event_unhandledrejection and https://github.com/jamestalmage/currently-unhandled.

Great. I will remove the error event part; it should be in an other PR or issue.

dinoboff · 2018-02-21T00:38:34Z

@novemberborn I got 100% coverage back but some test will keep catching handled rejection if they timeout I think.

novemberborn

some test will keep catching handled rejection if they timeout I think.

How do you mean?

I think we need two changes, and perhaps we should try-await-catch instead. What do you think?

novemberborn · 2018-02-21T11:39:28Z

test/_run.js

+
+		emitter.on('🦄', () => Promise.reject(first));
+		emitter.on('🦄', () => Promise.reject(second));
+		emitter.onAny(eventName => eventName !== 'error' && Promise.reject(third));


Don't need this eventName guard.

novemberborn · 2018-02-21T11:41:05Z

test/_run.js

@@ -390,4 +426,25 @@ module.exports = Emittery => {
 		const emitter = new Emittery();
 		t.throws(() => emitter.listenerCount(42), TypeError);
 	});
+
+	function catchUnhandledRejection(limit) {
+		return new PCancelable((onCancel, resolve) => {


PCancelable doesn't seem necessary.

I would have like to be able to cancel the unhandledRejection listener in case of failure, but it won't work with a cancellable promise. I will try something else.

You can't observe test failures though.

novemberborn · 2018-02-21T11:42:39Z

index.js

+			...staticListeners.map(listener => {
+				return listeners.has(listener) && new Promise(resolve => {
+					resolve(listener(eventData));
+				}).catch(hostReportError);


IIRC I switched this to new Promise(), but @dinoboff do you think we should use try-await-catch instead?

I prefer new Promise() in this case, but I don't mind.

Cool, neither do I.

I think it should use try-await-catch.

- catch listeners’ errors. - let the host emit an unhandled rejection event for each listener error.

sindresorhus · 2019-06-11T18:40:01Z

@dinoboff Interested in finishing this up? See https://github.com/sindresorhus/emittery/pull/19/files#r292604789.

@novemberborn Other than the above, does this look good to you?

sindresorhus · 2019-09-17T14:28:21Z

Ping :)

esclapes · 2019-10-17T21:29:09Z

Hey @sindresorhus, thanks a lot for this library it's being great using it.

I came up with a different approach to avoid losing any of the erred promises. It's easy enough to implement in user-space (see the example code below), but I wanted to share it with you as an alternative idea for you to consider. It basically does the following:

Calling .on() would wrap the passed listener callbacks so that we can try/catch any error thrown
all caught errors will be emitted using a custom (namespaced) event
consuming code can use a new onCallbackError method to get notified of these errors
a custom error is returned as the resolved value for Promise.all so that you could extract them with a simple static helper like Emitter.getErrors(resolvedValues). In the example below I am using verror but any custom error could do. Note that we are currently hapy with the callback and we have not implemented this last bullent point.

This is our wrapper implementing this approach:

const Emittery = require('emittery')
const VError = require('verror')

const EVENT_CALLBACK_ERROR = 'eventBus/callbackError'

const bus = new Emittery()

module.exports = {
  emit: bus.emit.bind(bus),
  clearListeners: bus.clearListeners.bind(bus),
  on(eventName, eventCallback) {
    bus.on(eventName, safelyWrapCallback(eventName, eventCallback))
  },
  onCallbackError(cb) {
    this.on(EVENT_CALLBACK_ERROR, cb)
  },
}

/**
 * Wraps the callback in an async function which will not
 * throw and will emit an EventBusCallbackError instead
 */
function safelyWrapCallback(eventName, cb) {
  const callBackName = cb.name || 'anonymous callback'

  return async function safelyWrappedCallback(payload) {
    try {
      return await Promise.resolve(cb(payload))
    } catch (cause) {
      const error = new VError(
        {
          name: 'EventBusCallbackError',
          cause,
          info: {
            eventName,
            callBackName,
          },
        },
        'Listener on %s rejected: %s',
        eventName,
        callBackName,
      )
      if (eventName !== EVENT_CALLBACK_ERROR) {
        // only emit for other eventNames to avoid recursion
        bus.emit(EVENT_CALLBACK_ERROR, { error })
      }
      return error
    }
  }
}

I'd love to hear your thoughts on this, and I'd be happy to contribute a PR. Thanks again!

EDIT: sorry to hijack this PR, I realize it would have been more appropriate to open a new issue.

sindresorhus · 2020-03-05T10:50:15Z

@esclapes Would be great if you could move this into a new issue.

sindresorhus · 2020-03-05T10:51:32Z

Closing this PR for lack of response. #51

novemberborn changed the title ~~[WIP] emit waits for all listeners to settle~~ emit() waits for all listeners to settle Jan 8, 2018

dinoboff closed this Jan 15, 2018

dinoboff reopened this Jan 15, 2018

dinoboff and others added 2 commits February 17, 2018 23:42

[WIP] emit waits for all listeners to settle

a554529

emit() waits for all listeners to settle

e15aca2

* The promise returned by emit() settles once all listeners do * If no listener threw an exception, the promise is fulfilled with `undefined` * Otherwise it's rejected with the first exception

dinoboff force-pushed the fix/emit-settlement branch from 3ba5cc4 to dddf500 Compare February 18, 2018 02:22

dinoboff force-pushed the fix/emit-settlement branch from dddf500 to 144540c Compare February 19, 2018 15:57

dinoboff force-pushed the fix/emit-settlement branch 2 times, most recently from 8501e60 to 636e275 Compare February 21, 2018 00:35

novemberborn requested changes Feb 21, 2018

View reviewed changes

emit never reject.

d08a21b

- catch listeners’ errors. - let the host emit an unhandled rejection event for each listener error.

dinoboff force-pushed the fix/emit-settlement branch from 636e275 to d08a21b Compare February 22, 2018 08:55

sindresorhus changed the title ~~emit() waits for all listeners to settle~~ Make emit() wait for all listeners to settle Jun 11, 2019

sindresorhus mentioned this pull request Mar 5, 2020

Make emit() wait for all listeners to settle #51

Open

sindresorhus closed this Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `emit()` wait for all listeners to settle #19

Make `emit()` wait for all listeners to settle #19

dinoboff commented Dec 17, 2017 •

edited

novemberborn commented Dec 29, 2017

novemberborn commented Jan 5, 2018

dinoboff commented Jan 5, 2018 •

edited

novemberborn commented Jan 6, 2018

novemberborn commented Jan 8, 2018

sindresorhus commented Jan 15, 2018

dinoboff commented Jan 15, 2018 •

edited

dinoboff commented Jan 15, 2018 •

edited

novemberborn commented Jan 15, 2018

novemberborn commented Jan 20, 2018

novemberborn commented Jan 21, 2018

sindresorhus commented Feb 17, 2018

dinoboff commented Feb 18, 2018

novemberborn commented Feb 18, 2018

novemberborn commented Feb 18, 2018

dinoboff commented Feb 19, 2018 •

edited

dinoboff commented Feb 19, 2018

novemberborn commented Feb 19, 2018

dinoboff commented Feb 20, 2018 •

edited

dinoboff commented Feb 21, 2018

novemberborn left a comment

novemberborn Feb 21, 2018

novemberborn Feb 21, 2018

dinoboff Feb 21, 2018

novemberborn Feb 21, 2018

novemberborn Feb 21, 2018

dinoboff Feb 21, 2018 •

edited

novemberborn Feb 21, 2018

sindresorhus Jun 11, 2019

sindresorhus commented Jun 11, 2019

sindresorhus commented Sep 17, 2019

esclapes commented Oct 17, 2019 •

edited

sindresorhus commented Mar 5, 2020

sindresorhus commented Mar 5, 2020

Make emit() wait for all listeners to settle #19

Make emit() wait for all listeners to settle #19

Conversation

dinoboff commented Dec 17, 2017 • edited

novemberborn commented Dec 29, 2017

novemberborn commented Jan 5, 2018

dinoboff commented Jan 5, 2018 • edited

novemberborn commented Jan 6, 2018

novemberborn commented Jan 8, 2018

sindresorhus commented Jan 15, 2018

dinoboff commented Jan 15, 2018 • edited

dinoboff commented Jan 15, 2018 • edited

novemberborn commented Jan 15, 2018

novemberborn commented Jan 20, 2018

novemberborn commented Jan 21, 2018

sindresorhus commented Feb 17, 2018

dinoboff commented Feb 18, 2018

novemberborn commented Feb 18, 2018

novemberborn commented Feb 18, 2018

dinoboff commented Feb 19, 2018 • edited

dinoboff commented Feb 19, 2018

novemberborn commented Feb 19, 2018

dinoboff commented Feb 20, 2018 • edited

dinoboff commented Feb 21, 2018

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn Feb 21, 2018

Choose a reason for hiding this comment

novemberborn Feb 21, 2018

Choose a reason for hiding this comment

dinoboff Feb 21, 2018

Choose a reason for hiding this comment

novemberborn Feb 21, 2018

Choose a reason for hiding this comment

novemberborn Feb 21, 2018

Choose a reason for hiding this comment

dinoboff Feb 21, 2018 • edited

Choose a reason for hiding this comment

novemberborn Feb 21, 2018

Choose a reason for hiding this comment

sindresorhus Jun 11, 2019

Choose a reason for hiding this comment

sindresorhus commented Jun 11, 2019

sindresorhus commented Sep 17, 2019

esclapes commented Oct 17, 2019 • edited

sindresorhus commented Mar 5, 2020

sindresorhus commented Mar 5, 2020

Make `emit()` wait for all listeners to settle #19

Make `emit()` wait for all listeners to settle #19

dinoboff commented Dec 17, 2017 •

edited

dinoboff commented Jan 5, 2018 •

edited

dinoboff commented Jan 15, 2018 •

edited

dinoboff commented Jan 15, 2018 •

edited

dinoboff commented Feb 19, 2018 •

edited

dinoboff commented Feb 20, 2018 •

edited

dinoboff Feb 21, 2018 •

edited

esclapes commented Oct 17, 2019 •

edited