- Notifications
You must be signed in to change notification settings - Fork360
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Ensure onStdinClose only completes for non-tty streams#1665
base:main
Are you sure you want to change the base?
Conversation
The initial stdin commit144cd35 inverted the TTY conditionalb/w the io/node and io/vm implementations of onStdinClose.The node implementation incorrectly checked for the presenceof TTY to complete the stream, while the vm implementationcorrectly checked for its absence. The commit that landedupstreamc7ab426 normalized the incorrect behaviour, whichmeans that sass was still not closing when stdin was closed,unless stdin was a TTY.Unfortunately that created a "worst of both worlds" situationbecause programs that start sass and then close unexpectedlywill still leave zombie sass processes running in thebackground, and wrapper scripts designed to mitigate thisexact problem will stop working because moving the process tothe background now incorrectly causes the job to stop.This change ensures we only complete the CancelableOperationonStdinClose for non-tty standard input streams.
Can you add a changelog entry as well? |
Thanks! Are you able to figure out why the Windows tests are failing? It seems unlikely to be a flake. |
@nex3 I can test Windows in a VM which is not ideal but it works :) I agree there's an issue and I will try to figure it out– right now running |
@nex3 Yes I think that's likely the best course. Other responsibilities have unfortunately taken precedence for the moment, but I would very much like to get back to this as time permits :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Marking as Request Changes pending the windows debugging.
9cff4ca to6c59213Compare
First, thank you so much for all the work to get#1411 merged! I realized only too late that there was a small typo in my initial version that has led to a bit of a gnarly bug. 😳
The initial stdin commit144cd35 inverted the TTY conditional
b/w the
io/nodeandio/vmimplementations ofonStdinClose.The node implementation incorrectly checked for the presence
of TTY to complete the stream, while the vm implementation
correctly checked for itsabsence. The commit that landed
upstreamc7ab426 normalized the incorrect behaviour, which
means that sass was still not closing when stdin was closed,
unless stdin was a TTY.
Unfortunately that created a "worst of both worlds" situation
because programs that start sass and then close unexpectedly
will still leave zombie sass processes running in the
background, and wrapper scripts designed to mitigate this
exact problem will stop working because moving the process to
the background now incorrectly causes the job to stop.
This change ensures we only complete the CancelableOperation
onStdinClose for non-tty standard input streams.