Skip to content

tty: enable buffering - #39253

Closed
ronag wants to merge 5 commits into
nodejs:masterfrom
nxtedition:stdio-hwm
Closed

tty: enable buffering#39253
ronag wants to merge 5 commits into
nodejs:masterfrom
nxtedition:stdio-hwm

Conversation

@ronag

@ronagronag commented Jul 3, 2021•
edited
Loading

Copy link
Copy Markdown
Member

HWM was set to 0 which would cause e.g. stdout.write(...) to
always return false.

Partly reverts 954217a

Refs: #39246

@ronag
tty: enable buffering
1dd2f48
HWM was set to 0 which would cause e.g. stdout.write(...) to
always return false.
Refs: nodejs#39246
@ronag
ronag requested a review from targosJuly 3, 2021 20:11
@github-actionsgithub-actionsBot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 3, 2021
@ronag

ronag commented Jul 3, 2021

Copy link
Copy Markdown
MemberAuthor

@targos not sure who's a good ping on this module?

@ronag

ronag commented Jul 3, 2021

Copy link
Copy Markdown
MemberAuthor

@nodejs/streams

@wa-Nadoo

wa-Nadoo commented Jul 3, 2021

Copy link
Copy Markdown

I think the highWaterMark removal from is_main_thread.js (954217a#diff-c09055446adf1d69b81595307e964da2121690a1d1bf2121845c9fcac0d86c86L152) should be reverted too. It was added years ago (bb56dcc#diff-36b7877cd8c72d2e5aa27243c142181d67d7be164927bae7503af6a720203aaeR500), and its removal can be pretty breaking.

@ronagronag added review wanted PRs that need reviews. stream Issues and PRs related to the stream subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 4, 2021
@nodejs-github-bot

nodejs-github-bot commented Jul 4, 2021

Copy link
Copy Markdown
Collaborator

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

addaleax
addaleax reviewed Jul 4, 2021
View reviewed changes
Comment threadlib/internal/bootstrap/switches/is_main_thread.js Outdated
@addaleaxaddaleax added semver-major PRs that contain breaking changes and should be released in the next major version. and removed dont-land-on-v12.x labels Jul 4, 2021
ronag
ronag commented Jul 5, 2021
View reviewed changes
Comment threadlib/internal/bootstrap/switches/is_main_thread.js Outdated
@ronag

ronag commented Jul 5, 2021

Copy link
Copy Markdown
MemberAuthor

@addaleax this is a partial revert. Isn't it better without the semver major label and instead don't land labels or what's the thought?

addaleax
addaleax reviewed Jul 5, 2021
View reviewed changes

@addaleaxaddaleax 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.

@ronag Yeah, I don’t see any issues with setting the HWM for the writable side here.

Comment threadlib/tty.js
@ronag
ronag requested review from addaleax and jasnellJuly 5, 2021 19:55
@nodejs-github-bot

nodejs-github-bot commented Jul 5, 2021

Copy link
Copy Markdown
Collaborator

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

addaleax
addaleax approved these changes Jul 6, 2021
View reviewed changes

@addaleaxaddaleax 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.

👍

benjamingr
benjamingr approved these changes Jul 6, 2021
View reviewed changes

@benjamingrbenjamingr 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.

good catch

@ronagronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v12.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jul 6, 2021
ronag added a commit that referenced this pull request Jul 7, 2021
@ronag
tty: enable buffering
a8a8638
HWM was set to 0 which would cause e.g. stdout.write(...) to
always return false.
Refs: #39246
PR-URL: #39253
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@ronag

ronag commented Jul 7, 2021

Copy link
Copy Markdown
MemberAuthor

Landed in a8a8638

@ronagronag closed this Jul 7, 2021
@BethGriggsBethGriggs mentioned this pull request Oct 14, 2021
Merged
2 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@targostargosAwaiting requested review from targos
@jasnelljasnellAwaiting requested review from jasnell

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.review wantedPRs that need reviews.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ronag@wa-Nadoo@nodejs-github-bot@addaleax@benjamingr