search

dart-lang / build

A build system for Dart written in Dart
https://pub.dev/packages/build
BSD 3-Clause "New" or "Revised" License
792 stars 207 forks source link

Build_runner should honor CTRL-c press during prompts #322

Open natebosch opened 7 years ago

natebosch commented 7 years ago

For example when prompting to delete existing files, CTRL-c should end the build.

natebosch commented 7 years ago

I think the VM tries to exit - we don't get an exception or anything - but something is keeping the VM running and we don't have a chance to safely close whatever it is we need to close.

jakemac53 commented 7 years ago

I think its because we synchronously block on reading stdin (https://github.com/dart-lang/build/blob/master/build_runner/lib/src/generate/build_impl.dart#L376), and we also swallow the terminate events (https://github.com/dart-lang/build/blob/master/build_runner/lib/src/generate/build.dart#L189) so we can wait for ongoing builds (although multiple events will just exit).

We never actually get into the streams handlers though since we are synchronously blocking on IO, so we never cancel our listener.

matanlurey commented 6 years ago

Since this isn't the MVP path, punting to M1+.

natebosch commented 6 years ago

I think we can solve this but we would have to consider it a breaking change.

The nice thing about stdin.readLineSync() is that it doesn't interfere with any other listeners on stdin (except for interception the next line so they don't see that input). The bad thing is that it is blocking so our SIGINT handler doesn't get a chance to run.

We can use sharedStdin from package:io and get the nextLine() in a non-blocking way - but we have to make a choice: 1 - Call SharedStdin.terminate() at the end of our work. This means that scripts wrapping build(), watch(), or run() can't listen() on stdin any time, and can only use sharedStdin before calling that method. This precludes spawning processes (like running tests) after the build in the same script. 2 - Document that callers to these methods must call SharedStdIn.terminate() at the end of their main() otherwise the VM will hang.

Whichever we choose we'd be breaking the ability to call stdin.listen() - other uses of stdin must go through sharedStdin in these wrapping scripts.

Note that the scripts wrapping run() and using the test command already essentially have the first case above.

matanlurey commented 6 years ago

For long-running test suites like Angular's _tests, this is a must. I got in a few cases today where I needed to use killall dart to get out of a hung terminal because ctrl-c did nothing. I think this should probably be M1, if possible.

natebosch commented 6 years ago

We'll need to see how it interacts with https://github.com/dart-lang/build/pull/934 if we try SharedStdIn

natebosch commented 6 years ago

There might be some places that package:test is intercepting things and getting us stuck as well - does angular have any integration tests that run build_runner from within a test?

Or maybe we need to be doing something different with our SIGINT handler before spawing the tests...

matanlurey commented 6 years ago

I don't think _tests runs the build_runner CLI, no.