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

On receiving SIGWINCH, only stdout has its rows/columns updated or resize event fired #2219

Closed
iarna opened this issue Jul 21, 2015 · 5 comments
Assignees
Labels
confirmed-bug Issues with confirmed bugs. process Issues and PRs related to the process subsystem.

Comments

@iarna
Copy link
Member

iarna commented Jul 21, 2015

I would expect that if both stdout and stderr are bound to /dev/tty then resizes would trigger on both tty objects, but this is not the case. This can be demonstrated with a simple script:

function setup (name, tty) {
  console.error(name, 'started with rows:', tty.rows, 'columns:', tty.columns)
  tty.on('resize', function () {
    console.error('\n',name, 'resized to rows:', tty.rows, 'columns:', tty.columns)
  })
}

setup('stdout', process.stdout)
setup('stderr', process.stderr)

console.error('sleeping for 10 seconds')
setTimeout(function () {}, 10000)

Run and try resizing the window before the timeout. You'll see update events from stdout but none from stderr.

@Fishrock123 Fishrock123 added the process Issues and PRs related to the process subsystem. label Jul 21, 2015
@Fishrock123
Copy link
Contributor

That is the case, yes, even clearly in the code: https://github.com/nodejs/io.js/blob/master/src/node.js#L626-L628

(Calls this: https://github.com/nodejs/io.js/blob/master/lib/tty.js#L76-L92)

And here it is from the blame'd commit: aad12d0#diff-6ff379484cbabad48301d485db111c08R329

I'm guessing it was just forgotten, since I assume stderr would have already existed.

@Fishrock123
Copy link
Contributor

@iarna if you'd like to make a PR it definitely seems reasonable to fix. :)

@Fishrock123 Fishrock123 added the confirmed-bug Issues with confirmed bugs. label Jul 21, 2015
@iarna
Copy link
Member Author

iarna commented Jul 21, 2015

oh hey that is pretty explicit huh =D

@Fishrock123 Fishrock123 self-assigned this Jul 23, 2015
Fishrock123 added a commit to Fishrock123/node that referenced this issue Jul 23, 2015
Fixes: nodejs#2219
PR-URL: nodejs#2231
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@Fishrock123
Copy link
Contributor

Fixed in bf2cd22, thanks for finding this!

@iarna would you need this to be backported to 0.12 / 0.10, or is it just an annoyance?

@iarna
Copy link
Member Author

iarna commented Jul 24, 2015

It's just an annoyance. I've worked around it by using stdout's info for now, which mostly works.

Obviously if stdout is redirected and stderr isn't it means we can't handle resize events for stderr. But having seen the actual code, I can do better than that. Having it fixed in 0.12 / 0.10 doesn't actually stop me from having to work around the bug, which is why I'm meh on back porting the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants