Skip to content
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

Revert "stream: error Duplex write/read if not writable/readable" #39246

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Jul 3, 2021

This reverts commit 954217a.

It breaks npm.

@github-actions github-actions bot 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
@targos
Copy link
Member Author

targos commented Jul 3, 2021

To reproduce the issue before this commit:

rm -rf tools/doc/node_modules
make doc

@targos
Copy link
Member Author

targos commented Jul 3, 2021

Here's the related code in npm that doesn't work as expected with the change:

https://github.com/npm/gauge/blob/e1839fb2fddb15b6e468ff5ec90086c3c59e084d/index.js#L226-L232

It seems that either the write method now returns false instead of true, or the 'drain' event is no longer emitted.
_writeTo is the process.stderr stream.

@ronag
Copy link
Member

ronag commented Jul 3, 2021

I’ll take a look tonight

@ronag
Copy link
Member

ronag commented Jul 3, 2021

@targos any possibility of a repro using just gauge?

@targos
Copy link
Member Author

targos commented Jul 3, 2021

@ronag

var Gauge = require('./deps/npm/node_modules/gauge/index.js');

var gauge = new Gauge(process.stderr, {
  theme: {hasColor: true},
  template: [
    {type: 'progressbar', length: 20},
    {type: 'activityIndicator', kerning: 1, length: 1},
    {type: 'section', default: ''},
    ':',
    {type: 'logline', kerning: 1, default: ''}
  ]
})

gauge.show("working…", 0)
setTimeout(() => { gauge.pulse(); gauge.show("working…", 0.25) }, 500)
setTimeout(() => { gauge.pulse(); gauge.show("working…", 0.50) }, 1000)
setTimeout(() => { gauge.pulse(); gauge.show("working…", 0.75) }, 1500)
setTimeout(() => { gauge.pulse(); gauge.show("working…", 0.99) }, 2000)
setTimeout(() => gauge.hide(), 2300)

@ronag
Copy link
Member

ronag commented Jul 3, 2021

There is a redraw live loop bug in gauge… I’m not sure what change triggers it. It unnecessarily calls redraw in drain. I’ll try and see if I can workaround it without changing gauge.

ronag added a commit to nxtedition/node that referenced this pull request Jul 3, 2021
HWM was set to 0 which would cause e.g. stdout.write(...) to
always return false.

Refs: nodejs#39246
@ronag ronag mentioned this pull request Jul 3, 2021
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Fixed in #39253

@targos targos closed this Jul 5, 2021
ronag added a commit that referenced this pull request Jul 7, 2021
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>
@targos targos deleted the revert-34385 branch July 25, 2021 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants