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

Abort sass if stdin is closed when watching #1411

Merged
merged 6 commits into from
Apr 7, 2022
Merged

Conversation

mcrumm
Copy link
Contributor

@mcrumm mcrumm commented Jul 29, 2021

Most command line tools (cat, fsevents, etc) and JS CLI watchers (postcss,
webpack, etc) listen to stdin and abort when the stdin is closed. This is
important because there is no guarantee the process who invokes sass will
terminate it, but it is guaranteed the stdin will be closed once the parent
dies.

This pull request listens to stdin when watching and exits accordingly,
in order to provide a user convenience and avoid zombie processes.

Note I chose to only listen when stdin is not a TTY: this is to maintain
the ability to background the watch process without unexpectedly closing it.
This means for instance you cannot use Ctrl+D to abort the watch, however,
when invoked by another process sass --watch will exit as expected.

For context, there is a some history wrt listening to stdin. Most recently
this feature was added to esbuild, wherein the PR comments @evanw provided a
great reference summary which I will repeat here for context (+ add a few more
examples).

PS– Thanks for dart-sass!

@jathak
Copy link
Member

jathak commented Aug 4, 2021

Thanks for this! We're definitely willing to merge this once the tests are passing.

@mcrumm
Copy link
Contributor Author

mcrumm commented Aug 4, 2021

@jathak Awesome! I was able to reproduce the failing tests on ubuntu. I rebased to get the latest changes and now everything is green locally :)

@jathak jathak requested a review from nex3 August 4, 2021 22:40
@nex3
Copy link
Contributor

nex3 commented Aug 5, 2021

Looks like the tests are still timing out.

@mcrumm
Copy link
Contributor Author

mcrumm commented Aug 6, 2021

Hi @nex3 – indeed, I spoke too soon :) I am happy to dive in deeper. I am new to Dart, but my naive impression is that maybe the exit in the stdin listener is racing another exit? Do you have any pointers on what to look for / changes to make?

@nex3
Copy link
Contributor

nex3 commented Aug 19, 2021

The proximate issue is that the tests are timing out while waiting for the process to exit. This is probably because the subscription to stdin is keeping the process alive when it would otherwise fall through the end of main().

Ideally, we'd avoid using exit() entirely and either explicitly close the watcher when stdin closes, or explicitly close the stdin subscription when the watcher closes. I'll see if I can come up with a good way to model that.

@nex3
Copy link
Contributor

nex3 commented Aug 19, 2021

dart-archive/async#194 adds some utilities to the async package that will make it much easier to model the concept of "cancel one operation when the other finishes and vice versa".

Specifically, we now close the watcher when stdin closes, using the
CancelableOperation API to represent both "a watcher that can be
closed" and "a subscription to stdin".
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.

3 participants