events: simplify on implementation #41851

ronag · 2022-02-04T12:59:56Z

Implement AsyncIterables using AsyncGenerator.

nodejs-github-bot · 2022-02-04T13:00:00Z

Review requested:

@nodejs/streams

benjamingr · 2022-02-04T13:01:56Z

test/parallel/test-events-on-async-iterator.js

@@ -113,39 +113,6 @@ async function throwInLoop() {
  assert.strictEqual(ee.listenerCount('error'), 0);
 }

-async function next() {


why these tests deleted?

They don't work with an async generator implementation. Please do have a look at it. But I can't find a way to make that test pass with an async generator and assuming async generators are "correct" then the test is "incorrect".

They don't work with an async generator implementation

What is the output with the async iterator implementation?

These tests specifically check that if the user asked for two events and then closed the iterator they still get those two events and not one event and then the iterator closing. That behavior is quite explicit - though we can talk about changing it (it'd be a breaking change though I doubt a lot of users are relying on this behavior)

So the problem here is the return() which doesn't actually guarantee that the last iterable.next() (where you expect { done: true }) will ever resolve since the async generator will never reach the yield statement while waiting for the promise to resolve. As far as I know there is not way to do this in an async generator. It's not a problem for for await loops so I'm not quite sure what exactly the spec or users would expect out of this use case?

Discussion starting roughly here: https://github.com/nodejs/node/pull/27994/files#diff-330bdd22a188135540523f69deb4f9563e03b00a913cd369e1ee84d899f178a9

Can't see? Bad link?

I don't know about the specifics of this but it seems weird to me that we want to achieve a behaviour which is not achievable with an async generator. Feels a bit like outside of spec. Maybe I'm a bit too hung up on assuming async generator is the "correct" way.

I don't know about the specifics of this but it seems weird to me that we want to achieve a behaviour which is not achievable with an async generator.

Not dropping these events is why the implementation didn't use an async generator initially - it's OK to change that if you think the simplification is worth losing that API guarantee.

It's true users who for... await will next() one at a time but implementations (think from) can ask for multiple values at once and this changes the behavior of the last chunks in the stream.

if you think the simplification is worth losing that API guarantee.

I think so. But I'm not sure if my opinion is sufficient here. I don't understand why we wanted this API guarantee to begin with.

vweevers · 2022-02-04T13:02:02Z

*simplify?

lib/events.js

vweevers · 2022-02-04T13:24:58Z

*simplify?

@ronag To clarify, I meant whether "simply" in the PR title and commit is a typo? If not, I don't understand the description

lib/events.js

benjamingr · 2022-02-04T14:13:24Z

lib/events.js

+        });
+      }
+    } finally {
+      eventTargetAgnosticRemoveListener(emitter, event, eventHandler);


So the behavior here is to drop all events when the iterator is returned right?

So if I .next() 5 times and then .return I would get { done: true, value: undefined } on all 5?

yes, I believe so

benjamingr · 2022-02-04T16:58:10Z

Any chance you can compare it with the implementation in https://github.com/nodejs/node/pull/41276/files in terms of performance?

e.g. make the changes in https://github.com/nodejs/node/pull/41276/files#diff-a22982bce4ecd41b8c79b8a6d66076ac7166d3cbd3e128585a9e3bc281a0a581R1327 to readline and then run the benchmark https://github.com/nodejs/node/pull/41276/files?show-viewed-files=true&file-filters%5B%5D=#

ronag · 2022-02-04T17:22:26Z

This isn't really for perf inc but here are the results relative to node 17.3.1

readline/readline-iterable.js n=10             ***    -11.37 %       ±5.09% ±6.84% ±9.02%
readline/readline-iterable.js n=100            ***      6.81 %       ±3.23% ±4.32% ±5.67%
readline/readline-iterable.js n=1000                    0.44 %       ±2.49% ±3.31% ±4.31%
readline/readline-iterable.js n=10000          ***      4.60 %       ±0.81% ±1.08% ±1.40%
readline/readline-iterable.js n=100000         ***      3.28 %       ±0.57% ±0.76% ±0.99%
readline/readline-iterable.js n=1000000        ***      1.06 %       ±0.53% ±0.71% ±0.92%

asd2

nodejs-github-bot · 2022-02-06T13:19:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42393/

nodejs-github-bot · 2022-02-07T17:45:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/42420/

aduh95 · 2022-02-11T11:44:36Z

This needs reviews.

benjamingr · 2022-02-11T12:14:45Z

@aduh95 the reason these are stalled is that there is a tradeoff here - this impl is simpler but slower and breaks several guarantees users might but probably don't rely on. The alternative is #41276

ronag · 2022-02-11T12:27:02Z

Yea, I openend this as I have concerns about the complexity of #41276. Was hoping to convince that given how simple it can be this is preferable.

ronag requested a review from benjamingr February 4, 2022 12:59

nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run. labels Feb 4, 2022

events: simply on implementation

9098447

ronag force-pushed the on-gen branch from aee6424 to 9098447 Compare February 4, 2022 13:00

benjamingr reviewed Feb 4, 2022

View reviewed changes

lib/events.js Outdated Show resolved Hide resolved

benjamingr reviewed Feb 4, 2022

View reviewed changes

lib/events.js Outdated Show resolved Hide resolved

ronag added 2 commits February 4, 2022 14:05

fixup

893e88b

fixup

fc717a8

ronag force-pushed the on-gen branch from 0ee29a9 to fc717a8 Compare February 4, 2022 13:16

fixup

d8decf1

ronag changed the title ~~events: simply on implementation~~ events: simplify on implementation Feb 4, 2022

ronag requested a review from mcollina February 4, 2022 13:35

benjamingr reviewed Feb 4, 2022

View reviewed changes

lib/events.js Show resolved Hide resolved

fixup

87008a0

benjamingr reviewed Feb 4, 2022

View reviewed changes

fixup

0eeb646

fixup

d68a516

addaleax reviewed Feb 5, 2022

View reviewed changes

asd2 Outdated Show resolved Hide resolved

ronag added 2 commits February 5, 2022 14:14

fixup

659d3d8

fixup

5e6c085

ronag added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Feb 6, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2022

ronag requested review from benjamingr and addaleax February 6, 2022 14:26

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2022

github-actions bot mentioned this pull request Feb 7, 2022

CI Reliability 2022-02-07 nodejs/reliability#195

Open

29 tasks

This was referenced Feb 8, 2022

CI Reliability 2022-02-08 nodejs/reliability#196

Open

CI Reliability 2022-02-09 nodejs/reliability#197

Open

CI Reliability 2022-02-10 nodejs/reliability#198

Open

CI Reliability 2022-02-11 nodejs/reliability#199

Open

aduh95 added review wanted PRs that need reviews. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 11, 2022

This was referenced Feb 12, 2022

CI Reliability 2022-02-12 nodejs/reliability#200

Open

CI Reliability 2022-02-13 nodejs/reliability#201

Open

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

ronag closed this Jan 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: simplify on implementation #41851

events: simplify on implementation #41851

ronag commented Feb 4, 2022 •

edited

Loading

nodejs-github-bot commented Feb 4, 2022

benjamingr Feb 4, 2022

ronag Feb 4, 2022 •

edited

Loading

benjamingr Feb 4, 2022

ronag Feb 4, 2022 •

edited

Loading

benjamingr Feb 4, 2022

ronag Feb 4, 2022 •

edited

Loading

ronag Feb 4, 2022 •

edited

Loading

benjamingr Feb 4, 2022

ronag Feb 4, 2022

vweevers commented Feb 4, 2022

vweevers commented Feb 4, 2022

benjamingr Feb 4, 2022

ronag Feb 4, 2022

benjamingr commented Feb 4, 2022

ronag commented Feb 4, 2022

nodejs-github-bot commented Feb 6, 2022

nodejs-github-bot commented Feb 7, 2022

aduh95 commented Feb 11, 2022

benjamingr commented Feb 11, 2022

ronag commented Feb 11, 2022 •

edited

Loading

events: simplify on implementation #41851

events: simplify on implementation #41851

Conversation

ronag commented Feb 4, 2022 • edited Loading

nodejs-github-bot commented Feb 4, 2022

benjamingr Feb 4, 2022

Choose a reason for hiding this comment

ronag Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

benjamingr Feb 4, 2022

Choose a reason for hiding this comment

ronag Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

benjamingr Feb 4, 2022

Choose a reason for hiding this comment

ronag Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

ronag Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

benjamingr Feb 4, 2022

Choose a reason for hiding this comment

ronag Feb 4, 2022

Choose a reason for hiding this comment

vweevers commented Feb 4, 2022

vweevers commented Feb 4, 2022

benjamingr Feb 4, 2022

Choose a reason for hiding this comment

ronag Feb 4, 2022

Choose a reason for hiding this comment

benjamingr commented Feb 4, 2022

ronag commented Feb 4, 2022

nodejs-github-bot commented Feb 6, 2022

nodejs-github-bot commented Feb 7, 2022

aduh95 commented Feb 11, 2022

benjamingr commented Feb 11, 2022

ronag commented Feb 11, 2022 • edited Loading

ronag commented Feb 4, 2022 •

edited

Loading

ronag Feb 4, 2022 •

edited

Loading

ronag Feb 4, 2022 •

edited

Loading

ronag Feb 4, 2022 •

edited

Loading

ronag Feb 4, 2022 •

edited

Loading

ronag commented Feb 11, 2022 •

edited

Loading