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

Ensure onStdinClose only completes for non-tty streams #1665

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mcrumm
Copy link
Contributor

@mcrumm mcrumm commented Apr 9, 2022

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 commit 144cd35 inverted the TTY conditional
b/w the io/node and io/vm implementations of onStdinClose.
The node implementation incorrectly checked for the presence
of TTY to complete the stream, while the vm implementation
correctly checked for its absence. The commit that landed
upstream c7ab426 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.

The initial stdin commit 144cd35 inverted the TTY conditional
b/w the io/node and io/vm implementations of onStdinClose.
The node implementation incorrectly checked for the presence
of TTY to complete the stream, while the vm implementation
correctly checked for its absence. The commit that landed
upstream c7ab426 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.
@nex3 nex3 self-requested a review April 11, 2022 20:31
@nex3
Copy link
Contributor

nex3 commented Apr 18, 2022

Can you add a changelog entry as well?

@nex3
Copy link
Contributor

nex3 commented Apr 18, 2022

Thanks! Are you able to figure out why the Windows tests are failing? It seems unlikely to be a flake.

@mcrumm
Copy link
Contributor Author

mcrumm commented Apr 19, 2022

@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 dart test from PowerShell hangs forever on a --watch test.

@nex3
Copy link
Contributor

nex3 commented May 16, 2022

@mcrumm If you don't have time to debug this for now, maybe we should roll back #1411 for now?

@mcrumm
Copy link
Contributor Author

mcrumm commented May 16, 2022

@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 :)

nex3 added a commit that referenced this pull request May 20, 2022
nex3 added a commit that referenced this pull request May 20, 2022
Copy link
Contributor

@nex3 nex3 left a 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.

@nex3 nex3 force-pushed the main branch 2 times, most recently from 9cff4ca to 6c59213 Compare July 21, 2023 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants