[Wilds] Handle SIGINT (^C) in ArcanistRuntime in a more formal way
ClosedPublic
Actions

Authored by epriestley on Sep 24 2018, 11:11 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13098: Plans: Arcanist toolsets and extensions

Commits

rARCa62c1d70dbd0: [Wilds] Handle SIGINT (^C) in ArcanistRuntime in a more formal way

Summary

Ref T13098. Add ^C handling and some small bits:

Update arc weld.
Test that arc weld filen<tab> completes filename (it does).
Add a "workflow stack" -- I plan to make it easier for arc diff to call arc unit / arc lint as formal sub-workflows, etc., and make "workflow X delegates to workflow Y" a more formal thing.

On interrupts:

Workflows can do something when you press ^C.
If they do, press ^C twice quickly to exit.
Otherwise, we exit on the first ^C.

The major thing I'd like to do in the short-ish term is to make phage report status on interrupt, but some other workflows might make sense to have interrupt handlers (maybe long-running stuff like arc upload / arc download) and third parties may find creative uses for them.

Test Plan

Added some sleep(...) to WeldWorkflow.
Interrupted, got program exit.
Added interrupt handlers and interrupted, got interrupt handling.
With interrupt handlers, interrupted twice. Got program exit.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

epriestley requested review of this revision.Sep 24 2018, 11:11 PM

epriestley created this revision.

Harbormaster failed remote builds in B20890: Diff 47076!Sep 24 2018, 11:11 PM

epriestley mentioned this in T13098: Plans: Arcanist toolsets and extensions.Sep 25 2018, 1:33 PM

amckinley added inline comments.Sep 25 2018, 7:36 PM

support/ArcanistRuntime.php
7	Rename this to `$lastInterruptTime`? (Or create an `ArcanistInterrupt` class with `$signo` and `$timeHandled` (and maybe `$handledBy`) instance variables). (And maybe initialize to 0; I had to think about the interval counting behavior below).
578–583	I might redefine `canHandleInterrupt` to take a `$signo` argument to avoid making subclasses reimplement the handlers from their parent that they don't want to handle. This is more to support extensions that want to do stuff like, uh.... register a signal handler that dumps `arc upload` progress to `/tmp/arc_progress` after receiving `USR1` or something.
596	Won't this get printed under lots of circumstances where `SIGINT` wasn't the actual signal though?

epriestley marked 3 inline comments as done.Sep 25 2018, 10:02 PM

epriestley added inline comments.

support/ArcanistRuntime.php
578–583	I think only USR1 and USR2 are even plausible, and since there's no way to send them from the keyboard (as far as I could tell?) it seems like a stretch to imagine that we'd ever use them for anything. I'll happily generalize this if you can identify any other signal that we can send from the keyboard and reasonably do something in response to, but last time I looked everything either can't be sent from the keyboard; or has some other important meaning already; or didn't seem to be something we can handle. In particular, we already handle SIGHUP (dump a stack trace; can't send from keyboard AFIAK), SIGTERM (we just terminate slightly more cleanly, don't know any way to send it from the keyboard) and SIGWINCH (as a legitimate signal when you resize your terminal window, updating terminal window metrics caches) elsewhere. ^Z sends a suspend signal but I didn't have any luck doing anything with it or preventing the suspension, as far as I can recall. I don't think USR1 and USR2 can be sent from the keyboard, or I couldn't figure out how at least. They could plausibly be used for something, but it's a stretch for me to imagine us building features based on "open a second terminal window, then run ps, then..."
596	Right now, since we only register for SIGINT with `pcntl_signal(SIGINT, ...)` above, this stuff will only be called for actual SIGINT.

amckinley added inline comments.Sep 25 2018, 10:32 PM

support/ArcanistRuntime.php
578–583	Before I do more research, have you seen the trick that dd uses to make it report progress? It’s literally “send USR1 from another terminal”: https://askubuntu.com/a/215521 I’m not arguing that that’s a super-brilliant and accessible design pattern, but there is some precedent for it.

wow

just wow

I definitely wasn't aware of anything in the wild actually doing this, much less something as "respectable" as dd

Rename lastInterrupt to lastInterruptTime for clarity.

AFAIK, these are the only signals that you can send from the keyboard:

⌃+c: SIGINT
⌃+\: SIGQUIT, which means "dump core" and could be aliased to SIGHUP.
⌃+Z: SIGTSTP, which, from the man pages:

Your program should handle this signal if you have a special need to leave files or system tables in a secure state when a process is stopped. For example, programs that turn off echoing should handle SIGTSTP so they can turn echoing back on before stopping.

This isn't a hill I'm ready to die on, but without too much imagination I could see someone writing an extension that at least wants to use USR1 or USR2. Making the API pass the signal numbers around would also make it easy to do stuff like this to express that the same function is handling multiple signals:

$this->installSignalHandler(SIGHUP, 'hupHandler');
$this->installSignalHandler(SIGQUIT, 'hupHandler');

Anyway, I'll accept this now, and if you're convinced, just put it back in Changes Planned.

This revision is now accepted and ready to land.Sep 26 2018, 5:59 PM

Just expanding on that:

I installed signal handlers for signals 1 through 31. On my machine, only 9 (KILL) and 17 (STOP) complained.

I sent the process every signal except 2 (INT), 9 (KILL) and 17 (STOP) from another terminal and the handler ran for all of them (look how good I am at Bash now):

X=( 1 3 4 5 6 7 8 10 11 12 13 14 15 16 18 19 20 21 22 23 24 25 26 27 28 29 30 31 ); for i in ${X[@]}; do echo sending signal $i; kill -$i 62723; sleep 1; done;

Actually pressing ^\ and ^Z on my keyboard in the same terminal also did useful things -- locally, these were signals 3 and 18 respectively. I may previously have conflated TSTP (^Z) and STOP (powerful magic).

It seems like using other signals for various purposes is more plausible than I'd concluded the last time I poked at this. It's possible that the PHP7 signal rewrite made some of this work better and that we legitimately had fewer options under PHP5, or I could have just gotten things wrong before or misremembered.

I'll update this to be parameterized on signo since this collectively puts other signals into the realm of plausibility for me.