-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
events: support once
with promise
#20909
Conversation
This commit introduces a `once` overload which returns a promise if a listener isn't passed to `once`. Fixes: nodejs#20893 PR-URL: Reviewed-By:
See #18646. Not 100% the same thing but close enough. Apropos the PR: overloading the method like this introduces potential for bugs if the caller thinks it's passing a function value when it's not. |
@bnoordhuis thanks, that PR seems to be about an unrelated feature (method to defer emits) - I'm actually -1 on adding that and +1 on adding this.
What do you think a more obvious-for-users behavior would be? What about still throwing if a second argument is passed (arguments.length > 1) but it is not a function? |
Previous attempt for something similar: #15204 |
@targos oh yeah - I guess the main difference in my opinion is that this PR uses clearer (IMO) naming which was a primary source of concern in the old PR. In addition, I have seen a year's worth more of users asking for this - we wanted to see if this is something people were doing and I became convinced we should do this. Also cc @jasnell |
Hi @benjamingr, first thank you for your interest in this feature and for opening this PR. I personally think it would make more sense to add a separate method to avoid confusion with the existing I also believe this should reject the returned Promise in case the "error" event is fired. const myEmitter = new EventEmitter()
function foo() {
setTimeout(() => myEmitter.emit('error', new Error('foo')), 500)
}
function bar() {
console.log('bar')
}
(async () => {
try {
foo()
await myEmitter.oncePromise('ready')
bar() // never gets called
} catch (err) {
console.log(err) // "foo"
}
})() As a reminder, on that point @benjamingr opinion was:
I agree event emitters are very generic, though the "error" event is definitely a special one. However when working with Promises you'd expect to be able to handle errors with the Promise API ( If error events are handled separately, I don't even see the point of using Promises at all. const myEmitter = new EventEmitter()
function foo() {
setTimeout(() => myEmitter.emit('error', new Error('foo')), 500)
}
function bar() {
console.log('bar')
}
(async () => {
try {
foo() // where should I handle the emitted error?
await myEmitter.oncePromise('ready') // this will await for ever
bar()
} catch (err) {
// we are losing this
}
})() Of course the event handlers should be removed once the Promise is fulfilled or rejected: const EventEmitter = require('events')
EventEmitter.prototype.oncePromise = function(completeEvent, errorEvent = 'error') {
return new Promise((resolve, reject) => {
const onComplete = (value) => {
this.removeListener(errorEvent, onError)
resolve(value)
}
const onError = (err) => {
this.removeListener(completeEvent, onComplete)
reject(err)
}
this.once(completeEvent, onComplete).once(errorEvent, onError)
})
} Note that I made the error event overridable here but I'm not even sure this is necessary. |
I am regretfully -1 on any PR with that sort of semantic. Namely because I've seen code like the below "in the wild" in production applications: var requests = new EventEmitter();
waitForAnalytic().catch(() => { /* explicitly ignore if this errors */ });
async function waitForAnalytic() {
await emitter.once('user-initialized'); // using a user module here since we don't have this yet
console.log('user initialized, reporting analytic');
// nothing business critical here
} If an "important" event is thrown, the code would swallow the error and in my opinion the behavior would be confusing to users. You can of course In general, I am -1 on adding event handlers for any event implicitly, I can see this working for streams (with Symbol.asyncIterator handling this) but not for a promisified Regarding the naming - the problem with the old name ( |
@benjamingr I don't care much about the name itself, I'm just saying it should be two different methods as the behavior I'm describing is indeed a bit different from the regular Concerning your example, how is that different from someone purposely swallowing error events?
Error swallowing is a bad practice, whatever it'd be vanilla exceptions, rejections or error events. Having a different method name would make the behavior difference easy to notice, and as long as that behavior is documented IMHO there's no problem. Now let's say we go with your approach. const myEmitter = new EventEmitter()
function foo() {
setTimeout(() => myEmitter.emit('error', new Error('foo')), 500)
}
function bar() {
console.log('bar')
}
(async () => {
try {
foo() // where should I handle the emitted error?
await Promise.all([
myEmitter.oncePromise('ready'),
myEmitter.oncePromise('error').then(err => Promise.reject(err))
])
bar()
} catch (err) {
console.error(err) // foo
}
})() I think such code would be un-intuitive and cumbersome. |
Good question. Mostly the expectation and explicitness. If someone would like to ignore errors I want it to be as explicit as possible. I don't want anything that doesn't make it obvious that it adds an
Well, in the above code you would get more correct behavior by not listening to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this doesn't feel quite right because we're just promisifying a single method/scenario.
However, even if we added this feature, I too believe we should not be housing it under an existing method. Having it under a different name would easily make this semver-minor I think. Currently it could be viewed as semver-major?
Well, for event emitters this is the single method/scenario that makes sense to promisify - streams deal with this better (as async iterators) but for event emitters anything other than I do hopefully we have Alternatively, we can also make an
I'm less concerned about semver-majorness vs. minorness - I'm more interested in figuring out whether other collaborators thing this is a good idea at all since I changed my mind about it. I don't think we talk about these things enough :)
Do you have any naming suggestions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 on polymorphic returns, way too much of a usability footgun.
If this is going to happen, a separate method would be preferred but even then I'm really questioning the need for this. I've looked into it several times in the past and could not come up with a good general use case that would justify it having to be in core. It's something that can easily be done in userland, there is no real performance benefit to be realized having it in core, and the use cases appear to be rather limited.
Marking semver-major because of the addition of the polymorphic return and the change to the error case. It may not be likely, but there is a non-zero chance of the polymorphic return breaking existing code. |
Well, users have been asking for it a lot which is why I thought it makes sense to bring it up again for discussion. The most common use case people are asking for is when they need to wait for a "I've started" event inside an
Well, it simplifies the API usage for every event emitter we ship and aligns it with how users write coded in practice. As for performance - I agree, it might be possible to motivate this with performance but this is not that PR.
Why do you believe that this is a foot gun? I'm honestly asking - I have no strong feelings either way. |
I'm not going to block it on the grounds that I don't think it's necessary. That's a generally subjective point-of-view. I'll remove my red-x if this were added as a separate method rather than implemented as a polymorphic return. |
Through hard experience I've learned that not every event emitter used in the ecosystem today extends from the By implementing as a separate method, we avoid changing the existing contract with EventEmitter.prototype.oncePromise = EventEmitter.prototype.oncePromise || function oncePromise() { /** ... **/ } |
I'm less concerned with whether or not this particular PR lands or not - I'm more interested in whether or not we should generally do this. If you believe this isn't useful or that the use cases above aren't interesting (or are but aren't worth it) do speak up. I don't have a horse in this race.
Got it, that makes sense. I hadn't considered this makes it significantly harder to polyfill. What about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m okay with this – it might be time for us to realize it’s more important that we have this feature in some way than what exactly the best way is.
I think once()
is a succinct name and I don’t really understand how this is semver-major, given that what we previously generated is a type checking exception.
@addaleax ... it's semver-major because it changes the contract of the existing method and breaks compatibility with, for example, things like eventemitter3, which has over 3+ million weekly downloads. Overloading |
To start some discussion regarding nodejs/node#20909 (comment)
I've made a PR at eventemitter3 to get some discussion started. I also have no issue with renaming to Going to wait for suggestions regarding the name from @jasnell and @mscdex as well as feedback from eventemitter3 (I am in no rush here) |
@benjamingr I don't have any suggestions for a new name, I am just saying we should not be housing this new functionality under the existing |
@mscdex thanks, you have raised two concerns - one regarding the name which I can resolve with waitFor and the other was
To which I responded in #20909 (comment) Could you please let me know if that addresses the concerns raised there? I’d ideally want users of Node to never have to write
I’m also not sure this PR is the right way to address my concern about it. |
@@ -79,3 +81,14 @@ common.expectsError(() => { | |||
EventEmitter.prototype.emit.apply(ee, args); | |||
} | |||
} | |||
|
|||
{ | |||
// verify the promise returning version of `once` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: // Verify the promise returning version of `once`.
(uppercase and period)?
Hi @benjamingr, Some answers on that comment (#20909 (comment)):
try {
client.connect()
await client.waitFor('connected', { errorEvents: [ 'serverError', 'readError' ] })
} catch (err) {
// handle my server error
} Once again my concern is we need a chance to handle errors using the Promise API. // what about the async flow wmwhen in a Promise chain or with await ?
client.on('error', (err) => handleError(err))
try {
client.connect()
await client.waitFor('connected') // pending forever
doOtherStuff();
} catch (err) {
// no way to actually use the catch construct
} How would you handle this kind of cases? emitter.on('error', (err) => {
handleError(err)
setTimeout(() => connect(), 500)
})
async function connect() {
try {
client.connect()
await client.waitFor('connected') // pending forever
doOtherStuff();
} catch (err) {
// no way to actually use the catch construct
}
} Promises are not cancellable so we have no control over them once they're created. Let's say we make 10 attempts before successfully connect to the server. That means we have 9 Promises that will be pending forever without any way of deleting them. |
@benjamingr That sounds like a user error, not something that could/should be solved in core? |
I would like to wait with renaming and landing this until after the collaborator summit if that's OK with everyone since I'm going to discuss some use cases which are related in the promises session. Afterwards my follow-up plan is to rename this (addressing mscdex and jasnell's concerns) and add some explanation in the PR from the discussion above motivating why expecting users to do @DaAitch thank you for weighing in on this issue and sorry for missing your comment earlier. I motivate why I believe this is needed in #20909 (comment) and the discussion following it. I am going to discuss various cases I've seen it implemented dangerously and indicate we need a much better async story in the summit too :) The fact you love promises but are concerned they are not low-level enough is very interesting to me. I would love to discuss why that is (you can send me an email or open an issue in https://github.com/nodejs/promise-use-cases/tree/master/use-cases ). I'm not sure I'd call promises a "best practice" or say that they're a higher level primitive than event emitters. I think they're just different, have different uses and different cases where they apply. I personally find promises useful and they are a language feature - so I think Node.js should try reasonably to behave in a way that doesn't make users take shortcuts since promises are hard and when people use them "manually" with |
@benjamingr Thank you for your answer. I think you mixed up my answer a bit.
Look at this eslint plugin and rule
That's exactly why something like an |
Thanks for following up:
This proposal is going to change a little and not called
Well, that's just a userland plugin - I happen to agree with its premise but it's one way to write code. I personally definitely think that a promise should be returned if it is created and we'll discuss it in the summit :)
Let me ask you a question - is it OK to have a synchronous function that never throws or doesn't throw? Promises parallel regular functions, a function that doesn't return a promise that rejects is like a function that never
I think we stop when users stop writing Note that we already convert some event emitters (readable streams for instance) to async iterators. |
@TimothyGu I agree with you that we need a way to catch error events, though IMO emitter.emit('error', new Error()) // from now on errorPromise is rejected for ever
// ...
const result = await Promise.race([
emitter.oncePromise('done'), // can't use oncePromise anymore
emitter.errorPromise
]); Instead, what would be more useful is: emitter.emit('error') // not related to any promise so far
// ...
const result = await Promise.race([
emitter.oncePromise('done'),
emitter.errorPromise() // reject on error events fired from now on
]); Possibly it would be even more useful to make errorPromise accept an event name as argument. However, in the (likely) case no error event is fired, each call to errorPromise will generate a pending promise that will never reject. I guess that could be considered a leak. Does anybody know the cost of pending promises? |
The problem isn't the cost of pending promises (they're virtually free) - the problem is that once we've added an |
@benjamingr Oh yeah I forgot that. It seems clear we only have 2 options:
I wonder what's the general opinion about these two options (@benjamingr is not a big fan of number 2). Anybody has an alternate version in mind? |
@kirly-af thanks for your comment. I'll try to clarify my position. I am trying to do something that's more useful for users. We have promises (awesome) but users use them with the promise constructor (lame) which they should never do. I don't think the promise constructor is to blame here - a different construction API would have different woes. I think manual construction of promises is inherently hard and that not many people get it right. I'm trying to solve what's most painful to users - I want to avoid cases where users have hard-to-debug surprises in their APIs. My objection is to any solution that interrupts with the fact event-emitter errors default to crashing Node.js. If I I would be interested in exploring paths that improve the error handling and debugging experience of our users. By the way - thanks for opening the issue here (and in bluebird before) and participating in the discussion, I appreciate it. |
BTW, I just found/read this stuff, and the ensuing related discussion evolved basically into my #21339, mod name only. (I filed the bug before finding this.) Oh, and also, this code very clearly leaks: emitter
.once("event", resolve)
.once("error", reject) Assuming only one of them is called (the common case), this will resolve/reject with that first one, but the second listener will still remain strongly linked to by the emitter, and thus the promise state will have to be retained, so the other function knows to ignore its input. @kirly-af With your two options, my #21339 is basically your second: combining the two into the same promise. I chose there to use @benjamingr Most cases where I've actually needed to await |
That can be done with This is explicitly for non-stream event emitters. |
@benjamingr I'm aware that's possible for readable streams (it doesn't flow, missing out on that optimization), but that's not so much the case with writable streams. And I'm specifically talking about
Awaiting at those two points just makes sense to me, and I'm already rolling my own abstraction repeatedly which is basically this proposal. |
I would be very interested in an abstraction that would improve the async story for writable streams - looking forward to see what you come up with! |
@benjamingr My now-closed-as-a-duplicate #21339 was pretty much that. A pragmatic solution to it I'm already using. |
I have to use something like this in userland for something that uses event emitters with promises: It's basically I think that promise-based .once alternative should support specifying events for accept and reject, else it won't be much useful — what's the point in awaiting a promise that could never fire (and emit an error instead)? @isiahmeadows I believe that the impl linked above doesn't leak. That doesn't fix the streams usecase, though — |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making it explicit, see concern above about this approach not supporting rejection (which could hang).
I like the direction where this goes, but would prefer to see something closer to .promise('end', 'error')
(possibly with support for specifying multiple events in an array).
@ChALkeR what sort of API would you like to see? What userland prior work can I research? |
@benjamingr I left a link in my comment above for an example. I am not aware of major userland right now. Copy-pasting here with some changes to support arrays (sorry for the poor formatting, typing it directly in a comment, also not tested): obj.promise = function (finish, error) {
if (!Array.isArray(finish)) {
if (finish === undefined) finish = [];
else if (typeof finish !== 'string') throw new Error('invalid arguments');
else finish = [ finish ];
}
if (!Array.isArray(error)) {
if (error === undefined) error = [];
else if (typeof error !== 'string') throw new Error('invalid arguments');
else error = [ error ];
}
if (finish.length + error.length === 0) throw new Error('invalid arguments');
return new Promise((accept, reject) => {
let acceptWrap = null, rejectWrap = null;
const cleanup = () => {
for (const event of finish) obj.removeListener(event, acceptWrap);
for (const event of error) obj.removeListener(event, rejectWrap);
};
acceptWrap = (...args) => { cleanup(); accept(...args); };
rejectWrap = (...args) => { cleanup(); reject(...args); };
for (const event of finish) obj.once(event, acceptWrap);
for (const event of error) obj.once(event, rejectWrap);
});
} |
In LoopBack, our primary usage of promisified const server = http.createServer(/*handler*/);
server.listen(80);
await pEvent(server, 'listening');
// ^^ resolves on success, rejects (throws) on error Similarly, one may want to wait until the server is stopped: server.close();
await pEvent(server, 'close');
// in most cases, we want to log and discard errors
// because there isn't much we can do here, right? IMHO, waiting for a server to start/stop listening is a common use case that should be kept in consideration when discussing a promisified |
What's the status on this one? |
Stalled is pretty accurate - I intend to revisit this in about a month |
What's the status on this? Edit: I guess that answers that. |
Hi @benjamingr |
@kirly-af in all honesty I have a lot going in my personal life (new job, sick family member, buying a house) and working on this is too much in terms of the emotional investment landing it would require. Doing this isn't too much work, I encourage other people to give this a shot and I can help guide people with it. Alternatively, I plan to get a lot more active again (coordinated with the job) around the summit (May) and I might tackle this again. |
This commit introduces a
once
overload which returns a promise if a listener isn't passed toonce
.This is a pretty common feature request and was constantly brought up when I asked users how we can improve our experience with event emitters for people using async/await.
I have decided to not add a new method but instead overload
.once
because it was safe to do so given the current behavior. Alternatives for this include adding aoncePromise
method.Ref: #20893
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes