events: add async emitter.when #15204

jasnell · 2017-09-05T15:24:10Z

Add async emitter.when() function that returns a Promise resolved with a one time handler

const promise = ee.when('foo');

promise.then((context) => {
  console.log(context.args);
}).catch((reason) => {});

ee.emit('foo', 1, 2, 3);

/cc @addaleax

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

events

mscdex · 2017-09-05T15:28:57Z

lib/events.js

+                                   'options', 'object');
+      }
+      // Do not pull these in at the top, they have to be lazy evaluated
+      const {


Is this the best/fastest way to pull these in (calling process.binding() every time)? What about making these top-level variables and adding a check if one of them is undefined and then set them from process.binding('util')?

mscdex · 2017-09-05T15:30:18Z

lib/events.js

+        resolved = true;
+        this.removeListener(type, listener);
+        promiseResolve(promise, { emitter: this, args });
+      }


Missing semicolon here and below for cancel?

mscdex · 2017-09-05T15:31:04Z

doc/api/events.md

@@ -586,6 +586,53 @@ to indicate an unlimited number of listeners.

 Returns a reference to the `EventEmitter`, so that calls can be chained.

+### async emitter.when(eventName[, options])


If it's just returning a Promise, do we really need to add the 'async ' prefix here? None of the examples even utilize async/await.

I was going back and forth on this. It depends on what (if any) convention we want to set on this. I'm not completely set on it and I expected that if anyone didn't like it then they'd speak up :-)

async/await works for any Promise right? If so, I'm not sure we need to explicitly add the prefix in that case.

I do like the async prefix.

I like the async prefix too but think that it's important we're consistent with the other method returning promises in the API (util.promisify)

addaleax · 2017-09-05T15:54:59Z

doc/api/events.md

+* `eventName` {any} The name of the event.
+* `options` {Object}
+  * `prepend` {boolean} True to prepend the handler used to resolve the
+    `Promise` to the handler queue.


Does this even make a difference since any handler is called asynchronously anyway?

Likely not. This is also something I've gone back and forth on so I'm happy to pull this back out if it doesn't make sense

FWIW, This would have an effect when registering multiple when Promises since those would be resolved in a specific order... e.g.

// `a` will always be printed before `b` ee.when('foo').then(() => console.log('b')); ee.when('foo', { prepend: true }).then(() => console.log('a')); ee.emit('foo');

// `b` will always be printed before `a` ee.when('foo').then(() => console.log('b')); ee.when('foo', { prepend: false }).then(() => console.log('a')); ee.emit('foo');

since those would be resolved in a specific order

Yes, and I would consider relying on promise resolution order a big anti-pattern ;)

I certainly cannot disagree with that :-)

addaleax · 2017-09-05T15:58:15Z

lib/events.js

+      const cancel = (eventName, removedListener) => {
+        if (!resolved && eventName === type && removedListener === listener) {
+          this.removeListener('removeListener', cancel);
+          promiseReject(promise, 'canceled');


I would argue that promise rejections should always be Error instances. Also, this should definitely contain the information that the Promise was rejected because its listener was removed – right now, it’s quite possible that a full-blown application using this might fail and canceled is literally all the diagnostic information it shows.

refack · 2017-09-05T15:59:19Z

I like the concept.

A few points to think about:

Currently EEs are sync, which is a nice feature. Promises handlers are explicitly async, so we lose the ability to synchronously interact with the event propagation.

Maybe wrap reason in an Error like in

node/lib/util.js

Lines 1061 to 1068 in ea2e636

    
           function callbackifyOnRejected(reason, cb) { 
        
             // `!reason` guard inspired by bluebird (Ref: https://goo.gl/t5IS6M). 
        
             // Because `null` is a special error value in callbacks which means "no error 
        
             // occurred", we error-wrap so the callback consumer can distinguish between 
        
             // "the promise rejected with null" or "the promise fulfilled with undefined". 
        
             if (!reason) { 
        
               const newReason = new errors.Error('ERR_FALSY_VALUE_REJECTION'); 
        
               newReason.reason = reason;

Maybe allow unregistering the listener with the promise:

const promise = ee.when('foo');
ee.removeListener('foo', promise);

or to keep the removeListener tight, use util.callbackify (needs implementing) to unwrap the actual listener.

const promise = ee.when('foo');
ee.removeListener('foo', util.callbackify(promise));

jasnell · 2017-09-05T16:01:51Z

@refack .. unregistering using the promise instance is difficult given that Promise chaining can easily cause the original promise reference to be lost. For instance, this wouldn't work, because the Promise the user has a reference to is not the original Promise. I implemented this already and decided against it because the ergonomics are a bit weird.

const promise = ee.when('foo').then(() => {});
ee.removeListener('foo', promise); // doesn't work!

addaleax · 2017-09-05T16:01:55Z

I don’t think adding a util.callbackify(Promise) overload is a good idea. If you want to be able to remove a listener, you can always just fall back to using callbacks.

refack · 2017-09-05T16:15:41Z

doc/api/events.md

+                     }
+                   });
+
+myEmitter.removeAllListeners();


Missing event name.

myEmitter.removeAllListeners('foo');

It's not needed in this case.

refack · 2017-09-05T16:17:32Z

In term of API design, the asymmetry with ee.on (single resolution vs requiring), makes me think that maybe the name should be whenOnce
using the myEmitter.removeAllListeners('foo'); with a promise rejection might trigger process.on('unhandledRejection'), while in other case could be a no side effect op.

unregistering using the promise instance is difficult given that Promise chaining can easily cause the original promise reference to be lost.

I don’t think adding a util.callbackify(Promise) overload is a good idea. If you want to be able to remove a listener, you can always just fall back to using callbacks.

Both ideas are "raw" thoughts, I see how they both can fail to cover all cases. I'm thinking of a case where this is exposed by a third party, and the end-user wants a simple solution that covers the simple solution.

Another option is to expose a remove/'cancel` method on the promise.

const promise = thirdPartyApi.whenAyncHandler('foo');
promise.remove();

jasnell · 2017-09-05T16:22:09Z

Regarding your item 4, given that a Promise can only ever be resolved at most once, there's never going to be a case where when would mean anything more than once, so I don't believe there's much value in making the name more complex.

For item 5, I actually consider that a feature. We could potentially make this an option tho... e.g.

ee.when('foo', { rejectOnRemove: false });

Add `async emitter.when()` function that returns a Promise resolved with a once handle

jasnell · 2017-09-05T16:31:30Z

@refack and @addaleax ... updated, PTAL

jasnell · 2017-09-05T17:39:45Z

@addaleax ... I've removed the prepend option.

YurySolovyov · 2017-09-05T19:34:58Z

Any idea if timeout option would be useful ?

jasnell · 2017-09-05T19:46:35Z

Potentially

YurySolovyov · 2017-09-05T19:49:37Z

Maybe via second argument options object?

jasnell · 2017-09-12T11:00:17Z

Ping @nodejs/collaborators

targos · 2017-09-12T11:38:47Z

test/parallel/test-event-emitter-when.js

+  ee.removeListener('foo', fn);
+}
+
+process.on('unhandledRejection', common.mustNotCall());


common.crashOnUnhandledRejection();

targos · 2017-09-12T11:54:49Z

This seems like an interesting feature but I'm afraid users will expect that you can do this:

var ee = new EventEmitter();
ee.when('myEvent').then((result) => {
  console.log(result);
}).catch((e) => { // catch the "error" event
  console.error('error!!!', e);
});
ee.emit('error', new Error('boom'));

Should we warn about that in the docs?

Also, do you have a concrete example where it would be useful to have this new method? It can be with a userland module.
I'm asking because as a user of Promises and async/await, I don't think I have ever felt the need for it.

eljefedelrodeodeljefe · 2017-09-12T12:03:27Z

As mentioned on twitter. Not a fan and -1. Is this really necessary to have in core? Seems fairly simple to do as module, no? My intention would be too keep surface area of the API small for maintenance, docs and bug tracker size reasons. I don't have strong feelings though.

benjamingr · 2017-09-12T12:09:07Z

Would appreciate pings on these issues in the future too :)

benjamingr · 2017-09-12T12:20:38Z

@targos

I'm asking because as a user of Promises and async/await, I don't think I have ever felt the need for it.

some pseudocode:

async function readFully(url) {
  var req = http.request({...});
  var buffer = ...
  req.on('data', (chunk) => buffer.concat(chunk));
  await req.wait('end');
  return buffer;
}

Basically, anywhere an event emitter has an end - which is streams.

refack · 2017-09-12T12:21:02Z

I was thinking about this and the use of promise.reject. After reading up on "Cancelable Promises" (~~ping @domenic~~) I don't think anymore that rejection should be the default consequent of removing the listener.

benjamingr · 2017-09-12T12:24:07Z

@refack Domenic has requested to not be pinged in this repository. (Not An Error is a really good read).

Cancellable promises is a rescinded proposal after consensus couldn't be reached on it.

I think we should do what we did with util.promisify and setTimeout - no cancellation if you do .when.

I'm more concerned by @targos's example in:

ee.when('myEvent').then((result) => {
  console.log(result);
}).catch((e) => { // catch the "error" event
  console.error('error!!!', e);
});
ee.emit('error', new Error('boom'));

People aren't great at realizing that promises are a one time thing. I think this means the naming needs to be clearer. Some bikeshed:

waitUntil (the DOM does this)
Overload once instead to return a promise if no callback is passed (this is nicer but a little risky)

refack · 2017-09-12T12:24:15Z

Basically, anywhere an event emitter has an end - which is streams.

@benjamingr, but there is the whole cancel concern... so:

async function* readFully(url) {
  var req = http.request({...});
  var buffer = ...
  req.on('data', (chunk) => buffer.concat(chunk));
  try {
    await req.wait('end');
  } catch (r) {
    if (r.code !== 'ERR_EVENTS_WHEN_CANCELED') throw e;
  }
  return buffer;
}

benjamingr · 2017-09-12T12:26:54Z

@refack that's a good point, in practice "promisifying" streams is a little harder. I think it makes sense for .wait to reject when an error is emitted from the emitter (that is, wait for any event implicitly adds an error event handler) but that's risky too.

eljefedelrodeodeljefe · 2017-09-12T12:28:26Z

See let alone those discussions make me wanna reject this.

benjamingr · 2017-09-12T12:30:06Z

@eljefedelrodeodeljefe I tend to agree, but no one suggested merging it yet - there is a good chance there are good solutions to all these problems we're discussing.

This is the first time we're really talking about adding a promise based API to Node.js and this is a really tricky one.

targos · 2017-09-12T12:33:15Z

I think it makes sense for .wait to reject when an error is emitted from the emitter (that is, wait for any event implicitly adds an error event handler) but that's risky too.

If you don't add an implicit error event handler, the user would have to do things like:

await Promise.race([
  req.wait('end'),
  req.wait('error').then((r) => {throw r.args[0];})
]);

That's not very nice :(

mcollina · 2017-09-12T13:49:18Z

Before landing util.promisify, there have been dozens of modules to promisify err-back function. However, this proposal is the first of its kind (unless I am wrong, in case please point me to prior art).

How about we release this as module on npm, do a couple of blog posts, see if the ecosystem likes it and then decides? We can figure out the details on how to do these easily on npm-land, while once it lands here it is very hard to change.

How about:

const oncep = require('oncep')

oncep(ee, 'myevent').then(...).catch(...)

jasnell · 2017-09-12T14:39:21Z

Closing given the feedback.

benjamingr · 2017-09-12T14:42:41Z

Before landing util.promisify, there have been dozens of modules to promisify err-back function. However, this proposal is the first of its kind (unless I am wrong, in case please point me to prior art).

The DOM actually does this (waitUntil I think?), and people have asked us dozens of times (on StackOverflow and on GH) how to promisify event emitters.

mcollina · 2017-09-12T14:49:48Z

@benjamingr oh I did not know. Let's work out a solution and test it in the ecosystem before landing here. There are so many edge cases it might take several iterations to do properly.

benjamingr · 2017-09-12T14:54:15Z

@mcollina I agree, I think this is pretty hard. To be honest I think that once async iterators land and are optimized they provide a nicer alternative to what this does:

const promise = AsyncIterator.from(ee, 'foo').next(); // I made this syntax up, but you get the gist

Since async iterators are compatible with consuming streams and the DOM already uses them. It would be nice to do something like:

async function once(stream) {
  for await(const item of stream) return item;  
}

pemrouz · 2017-09-12T19:34:07Z

Some prior art: https://github.com/utilise/emitterify/blob/master/README.md#emitterify

eljefedelrodeodeljefe · 2017-09-12T21:38:29Z

Thank you for the discourse.

mcollina · 2017-09-12T21:39:09Z

@benjamingr I don't think AsyncIterators are a good solution for this. AsyncIterator needs backpressure support to avoid memory issues (like streams).

Qard · 2017-09-12T23:14:42Z

Is the await for each item not sufficient to provide backpressure?

mcollina · 2017-09-13T06:23:27Z

@Qard no. EventEmitter has no concept of control flow and buffering, in fact it's what we use to implement those.

Qard · 2017-09-13T06:29:32Z

Wouldn't readable+read work? I'm a bit detached from the goings on of core streams work, but I wrote my own userland thing that seems to work fine with readable+read.

mcollina · 2017-09-13T06:35:49Z

Yes because of the semantics of read(). That is not part of EE.

Qard · 2017-09-13T06:42:51Z

Not sure what EE has to do with AsyncIterator backpressure. I think I'm missing something. 😕

mcollina · 2017-09-13T06:49:46Z

@Qard I am referring to the example in #15204 (comment) (the first one)

benjamingr · 2017-09-13T10:04:13Z

@mcollina in genera you're right but returning from a for await loop closes the iterator which should cause it to buffer at most one item before not listening to items anymore (or am I misunderstanding?)

benjamingr · 2017-09-13T10:05:25Z

Oh never mind, you're referring to the first example. A correct way to do it would be to call .return on the async iterator after the next completes. For... await is much nicer here.

mcollina · 2017-09-13T10:10:46Z

Regarding the For..await loop, the problem is that the iterator cannot stop the events being emitted. So, if you are nesting another await call, you will be filling up memory.
As an example, by using a similar construct on the 'data' event, you will be filling up memory and not signal upstream to slow down, the 'data' events will be waiting inside the iterator.

Streams are built to handle that case, and it is good to add AsyncIterator there. Because Streams are EventEmitters, it does not feel correct that the behavior is in both and it might create some contention.

benjamingr · 2017-09-13T10:38:11Z

Regarding the For..await loop, the problem is that the iterator cannot stop the events being emitted.

You mean for a general event emitter case? In that case I apologize for misunderstanding and agree. AsyncIterators only make sense as a translation from streams. The look exiting (by the return) calls .return on the AsyncIterator which can (and I think it should but I'm really not sure) close the stream.

AsyncIterators are actually very good at backpressure because they are lazy and pull based, the consumer tells the producer exactly how much data they need. I recall @headinthebox had some interesting writeup on it but I can't find it.

I'll see if I can get people to use async iterators with streams (and the package from the issue) when I get back from traveling (Mid October) to see if I can get more data on what works.

mcollina · 2017-09-13T10:43:51Z

@benjamingr we are in agreement.

refack · 2017-09-29T17:24:23Z

Well WHATWG landed AbortController whatwg/fetch#523:

const controller = new AbortController();
const signal = controller.signal;

setTimeout(() => controller.abort(), 5000);

fetch(url, { signal }).then(response => {
  return response.text();
}).then(text => {
  console.log(text);
}).catch(err => {
  if (err.name === 'AbortError') {
    console.log('Fetch aborted');
  } else {
    console.error('Uh oh, an error!', err);
  }
});

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Sep 5, 2017

jasnell requested a review from addaleax September 5, 2017 15:24

mscdex reviewed Sep 5, 2017

View reviewed changes

addaleax reviewed Sep 5, 2017

View reviewed changes

refack reviewed Sep 5, 2017

View reviewed changes

jasnell added 4 commits September 5, 2017 09:25

events: add async emitter.when

b10cea3

Add `async emitter.when()` function that returns a Promise resolved with a once handle

[Squash] Nits

2888ab1

[Squash] Use Error for reason

c02d332

[Squash] Add error code to docs

efd97d2

jasnell force-pushed the events-when branch from 115b6e9 to efd97d2 Compare September 5, 2017 16:30

[Squash] Remove prepend option

438e695

targos reviewed Sep 12, 2017

View reviewed changes

jasnell closed this Sep 12, 2017

refack mentioned this pull request Sep 29, 2017

meta: planning for promisified fs and crypto #15413

Closed

targos mentioned this pull request May 23, 2018

events: support once with promise #20909

Closed

4 tasks

		@@ -586,6 +586,53 @@ to indicate an unlimited number of listeners.

		Returns a reference to the `EventEmitter`, so that calls can be chained.

		### async emitter.when(eventName[, options])

events: add async emitter.when #15204

events: add async emitter.when #15204

Conversation

jasnell commented Sep 5, 2017

Checklist

Affected core subsystem(s)

mscdex Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Sep 5, 2017

jasnell commented Sep 5, 2017

addaleax commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Sep 5, 2017 • edited Loading

jasnell commented Sep 5, 2017

jasnell commented Sep 5, 2017

jasnell commented Sep 5, 2017

YurySolovyov commented Sep 5, 2017

jasnell commented Sep 5, 2017

YurySolovyov commented Sep 5, 2017

jasnell commented Sep 12, 2017

targos Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

targos commented Sep 12, 2017

eljefedelrodeodeljefe commented Sep 12, 2017

benjamingr commented Sep 12, 2017

benjamingr commented Sep 12, 2017 • edited Loading

refack commented Sep 12, 2017 • edited Loading

benjamingr commented Sep 12, 2017

refack commented Sep 12, 2017

benjamingr commented Sep 12, 2017 • edited Loading

eljefedelrodeodeljefe commented Sep 12, 2017

benjamingr commented Sep 12, 2017

targos commented Sep 12, 2017

mcollina commented Sep 12, 2017

jasnell commented Sep 12, 2017

benjamingr commented Sep 12, 2017 • edited Loading

mcollina commented Sep 12, 2017

benjamingr commented Sep 12, 2017

pemrouz commented Sep 12, 2017

eljefedelrodeodeljefe commented Sep 12, 2017

mcollina commented Sep 12, 2017

Qard commented Sep 12, 2017

mcollina commented Sep 13, 2017

Qard commented Sep 13, 2017

mcollina commented Sep 13, 2017

Qard commented Sep 13, 2017

mcollina commented Sep 13, 2017 • edited Loading

benjamingr commented Sep 13, 2017

benjamingr commented Sep 13, 2017

mcollina commented Sep 13, 2017

benjamingr commented Sep 13, 2017

mcollina commented Sep 13, 2017

refack commented Sep 29, 2017

mscdex Sep 5, 2017 •

edited

Loading

refack commented Sep 5, 2017 •

edited

Loading

targos Sep 12, 2017 •

edited

Loading

benjamingr commented Sep 12, 2017 •

edited

Loading

refack commented Sep 12, 2017 •

edited

Loading

benjamingr commented Sep 12, 2017 •

edited

Loading

benjamingr commented Sep 12, 2017 •

edited

Loading

mcollina commented Sep 13, 2017 •

edited

Loading