FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

readline: buffer line events in async iterator by benjamingr · Pull Request #41708 · nodejs/node · GitHub

/ node Public

readline: buffer line events in async iterator - #41708

Closed
benjamingr wants to merge 3 commits into
nodejs:mainfrom
benjamingr:fix-readline-lost-lines
ConversationCommits (3)ChecksFiles changed
Closed

readline: buffer line events in async iterator#41708
benjamingr wants to merge 3 commits into
nodejs:mainfrom
benjamingr:fix-readline-lost-lines

Conversation

benjamingr commented Jan 26, 2022

Copy link
Copy Markdown
Member

Beforehand the async iterator for readline was created lazily when the
stream was accessed which meant no events were buffered. In addition
stream handling was modernized and improved in the process to support
backpressure correctly. A test was added to assert this.

There is a cost here (the stream is always created) I believe it is well worth it to enable modern usage of readline without dropping lines.

There is also an unrelated bugfix here (we return the wrong thing from events.on(...).throw(...).

cc @nodejs/readline @nodejs/streams @jfriend00

Fixes: #28565
Fixes: #33463

readline: buffer line events in async iterator
5341413 
Beforehand the async iterator for readline was created lazily when the
stream was accessed which meant no events were buffered. In addition
stream handling was modernized and improved in the process to support
backpressure correctly. A test was added to assert this.

Fixes: nodejs#28565
Fixes: nodejs#33463
fixup!
073dbfd
nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module. labels Jan 26, 2022
benjamingr commented Jan 26, 2022
View reviewed changes
Comment thread lib/internal/readline/interface.js
const onIterator = on(this, 'line');
this.once('close', () => onIterator.return());
const readable = Readable.from(onIterator).map((line) => line[0]);
this[kLineObjectStream] = readable;

benjamingr Jan 26, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Note the current implementation does not close the readline.Interface when the async iterable is closed. This can be easily fixed if we want by doing readable.once('close', () => this.close()) though I am not sure which behavior is better.

mcollina Jan 27, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it should close it. The pattern for async iterators is that the stream is fully consumed after the loop. Maybe add an option?

benjamingr Jan 27, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it should close it too but was hesitant to change the existing behavior. If you agree it should be closed I'll close it.

mcollina Jan 29, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think so.

mcollina approved these changes Jan 26, 2022
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

benjamingr commented Jan 27, 2022

Copy link
Copy Markdown
Member Author

(Note to self: This also needs a doc update to remove the sentence about not attaching the listener early enough)

benjamingr commented Jan 27, 2022

Copy link
Copy Markdown
Member Author

The failing for backpressure test looks kind of wrong I think?

It wraps the events with a readable settings its highWaterMark to 16 (default for objectMode) and stops asking for data once we buffered 16 items by calling pause.

This means that other consumers of the readline.Interface won't get line events they're listening to which is kind of confusing behavior I think? I think for the very least we should check no one else is listening to line before pausing.

this is like one consumer in a multicast system pausing the broadcast for everyone because it can't buffer enough.

Alternatively I'm not sure backpressure makes a ton of sense here? I'll consider

mcollina commented Jan 27, 2022

Copy link
Copy Markdown
Member

The failing for backpressure test looks kind of wrong I think?

I'm probably lost, could you link?

This means that other consumers of the readline.Interface won't get line events they're listening to which is kind of confusing behavior I think? I think for the very least we should check no one else is listening to line before pausing.

If you are using an async iterator you are the primary consumer of that data. This should probably documented somewhere but a lot of unwanted things happen when multiple consumers are attached.

benjamingr commented Jan 27, 2022

Copy link
Copy Markdown
Member Author

I'm probably lost, could you link?

There is a test that checks that we .pause the readline.Interface after the readable associated with the async iterator reaches its watermark (which is a non-configurable 16 lines since that's the default for objectMode).

You can see the test in https://github.com/nodejs/node/blob/master/test/parallel/test-readline-async-iterators-backpressure.js

It's kind of confusing since if errors happen those will be buffered too in the async iterable which is why this behavior changed in from: #29428 .

This isn't too hard to "fix" since from already sets reading = false when the readable has buffered enough - it just defaults to a highWaterMark of 1 and not 16.

benjamingr commented Jan 28, 2022

Copy link
Copy Markdown
Member Author

I think I'll split this PR into two - one just fixing the bug and another to discuss backpressure to make the discussion easier - hope that's ok?

mcollina commented Jan 28, 2022

Copy link
Copy Markdown
Member

go for it

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 28, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 28, 2022

nodejs-github-bot commented Jan 28, 2022

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Jan 29, 2022
Open
22 tasks
benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022

nodejs-github-bot commented Jan 29, 2022

Copy link
Copy Markdown
Collaborator

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

This was referenced Jan 30, 2022
Open
Open
Open
Open
benjamingr added the blocked PRs that are blocked by other issues or PRs. label Feb 2, 2022

benjamingr commented Feb 2, 2022 •
edited by aduh95
Loading

Copy link
Copy Markdown
Member Author

I want to wait for #41276 to land first

Mifrill reviewed Nov 28, 2023
View reviewed changes
Comment thread test/parallel/test-readline-buffers.mjs

await setImmediate();
const result = [];
for await (const item of rl) {

Mifrill Nov 28, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It seems like it's redundant await, isn't it?

aduh95 commented May 11, 2024

Copy link
Copy Markdown
Contributor

This needs a rebase/

benjamingr closed this Jun 27, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
+1 more reviewer
Mifrill Mifrill left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

blocked PRs that are blocked by other issues or PRs. events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module.

Projects

None yet

Milestone

No milestone

6 participants

Back | FazBrowse Home | New Git URL