Signal handling fixes #10758

gshuflin · 2020-09-11T00:25:21Z

Problem

Ideally, pants should exit promptly when a user hits Ctrl-C in the terminal, regardless of how it is configured to run or what it is running. cf. #10051 and other general observations made while using pants, there are still a few cases where pants does not respond quickly to a Ctrl-C and hangs or otherwise takes a while to quit. This commit is an attempt to fix these cases.

Solution

In the --debug case, the problem turned out to be that we were explicitly disabling SIGINT handling in the case where pants was running an InteractiveProcess. For the non-pantsd case, this is desirable - we use the InteractiveProcess abstraction for test --debug and a few other cases where we legitimately want a pants subprocess to take over the terminal and do its own signal handling. But when pantsd is running, we don't want to explicitly disable SIGINT handling, because that means that pantsd will ignore a signal from its own client process. This commit solves the problem by adding a flag to SignalHandler to indicate whether this is a pantsd run, and doesn't ignore SIGINT if it is.

Some other instances of pants ignoring prompt signal handling are caused by rust code not heeding the Python interrupt state triggered by SIGINT, resulting in pants not quitting until execution returns to the Python main thread. This commit works around this by having the Python signal handling infrastructure set a flag on ExceptionSink once it has handled a signal that would cause pants to terminate. A python helper function is passed into the main execution loop in scheduler.rs that checks for the presence of this flag, and breaks the loop to terminate the engine if it finds that the flag is set.

Result

Ctrl-C should now work in the test --debug case with pantsd, and work in the --no-pantsd case when any kind of test is running. It still does seem to take more than one hit of Ctrl-C to fully quit pants in some cases, though, which will be addressed in followup commits.

stuhood

Thanks!

stuhood · 2020-09-11T17:22:25Z

src/rust/engine/src/scheduler.rs

@@ -474,6 +476,19 @@ impl Scheduler {
    session.maybe_display_initialize(&self.core.executor, &sender);
    self.execute_helper(request, session, sender);
    let result = loop {
+      match externs::call(&python_signal, &[]) {


Would be good to move this out into a helper function to keep the loop body small. You might also move the check "after" the call to recv_timeout but before the match... as the most likely time to receive a signal is while we are waiting, and reacting to it before we render the UI/print_stderr/etc will be the most responsive.

stuhood · 2020-09-11T17:24:47Z

src/rust/engine/src/scheduler.rs

+            return Err(ExecutionTermination::KeyboardInterrupt);
+          }
+        }
+        Err(e) => {


If this function is running in the main thread (it won't always be), it's possible that the error type received here will actually be KeyboardInterrupt: in which case you'd want to return ExecutionTermination::KeyboardInterrupt.

coveralls · 2020-09-14T17:33:18Z

Coverage remained the same at 0.0% when pulling 3e3f452 on gshuflin:pantsd_signal_handling2 into fa71675 on pantsbuild:master.

stuhood

Thanks!

stuhood · 2020-09-14T21:31:24Z

src/python/pants/base/exception_sink.py

-    # An instance of `SignalHandler` which is invoked to handle a static set of specific
-    # nonfatal signals (these signal handlers are allowed to make pants exit, but unlike SIGSEGV they
-    # don't need to exit immediately).


This information was potentially useful... did you determine that it is no longer accurate?

Nope that was an extraneous delete, restoring

stuhood · 2020-09-14T21:33:29Z

src/python/pants/engine/process.py

+        ExceptionSink.toggle_ignoring_sigint(True)
        return self._scheduler.run_local_interactive_process(request)


It seems like this would be a good place to use a context manager / with block... any idea why we stopped doing that?

This code never used a with block, although I agree that that would be a good improvement, and I might implement that shortly in a followup commit.

stuhood · 2020-09-14T21:35:44Z

src/rust/engine/src/externs/mod.rs

+
+pub fn call(func: &Value, args: &[Value]) -> Result<Value, Failure> {
+  let output = call_function(func, args);
+  let gil = Python::acquire_gil();


Could avoid re-acquiring the GIL here unless this fails, by moving let gil inside the map_err.

As well as do some additional refactoring. [ci skip-rust] [ci skip-build-wheels]

[ci skip-build-wheels]

Eric-Arellano · 2020-09-15T06:11:45Z

Yay, ./pants --no-pantsd repl and ./pants --no-pantsd repl --shell=ipython behave identically to running without Pants!

▶ ./pants --no-pantsd repl
Python 3.6.10 (default, May 12 2020, 19:47:44)
[GCC 4.2.1 Compatible Apple LLVM 11.0.0 (clang-1100.0.33.8)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> [ctrl-c]
KeyboardInterrupt
>>> [ctrl-d]
now exiting InteractiveConsole...

▶ ./pants --no-pantsd repl --shell=ipython
23:05:07.69 [INFO] Completed: Building ipython.pex with 1 requirement: ipython==7.16.1
Python 3.6.10 (default, May 12 2020, 19:47:44)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.16.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: [ctrl-c ignored; ctrl-d]
Do you really want to exit ([y]/n)? y

Pantsd isn't quite right, but an improvement. If you use ctrl-c, it will immediately break out of the process. It would be better to do the KeyboardInterrupt like you do without Pantsd and without Pants, but this seems acceptable.

ctrl-d works on a clean run. But then there's a weird bug. Reproduce with:

Run ./pants repl. Use ctrl-c. Should exit fine.
Run ./pants repl again. It should immediately start up the process for you. Now do ctrl-d or ctrl-c. Either you use, the process will not exit cleanly:

▶ ./pants repl
>>> [ctrl-c]

code/projects/pants  master ✔                                             29m  ⍉
▶ O

code/projects/pants  master ✔                                             29m  ⍉
▶ O

I can't delete the O, and when I type something, the characters get corrupted.

gshuflin force-pushed the pantsd_signal_handling2 branch from 0bbeb86 to cf88745 Compare September 11, 2020 03:43

gshuflin marked this pull request as ready for review September 11, 2020 04:22

stuhood approved these changes Sep 11, 2020

View reviewed changes

gshuflin force-pushed the pantsd_signal_handling2 branch 2 times, most recently from 875acca to abcebc8 Compare September 14, 2020 17:00

gshuflin force-pushed the pantsd_signal_handling2 branch from abcebc8 to 4dabde2 Compare September 14, 2020 21:26

stuhood approved these changes Sep 14, 2020

View reviewed changes

gshuflin added 8 commits September 14, 2020 15:46

Modify SignalHandler to shut down --debug tests correctly

2207ac6

As well as do some additional refactoring. [ci skip-rust] [ci skip-build-wheels]

explicit signal handling check

658219d

[ci skip-build-wheels]

Clarify ctrl-c message for pantsd

0d881be

[ci skip-build-wheels]

Move loop-break logic to helper fn

da06af1

[ci skip-build-wheels]

check for KeyboardInterrupt exception

0091678

[ci skip-build-wheels]

Fix test

0ff1145

[ci skip-build-wheels]

Remove test for I/O on closed file

2aac092

[ci skip-build-wheels]

PR feedback

3e3f452

[ci skip-build-wheels]

gshuflin force-pushed the pantsd_signal_handling2 branch from 97264c8 to 3e3f452 Compare September 14, 2020 22:46

gshuflin merged commit bfd65d8 into pantsbuild:master Sep 14, 2020

gshuflin deleted the pantsd_signal_handling2 branch September 14, 2020 23:54

Eric-Arellano mentioned this pull request Sep 15, 2020

pants hangs forever waiting for pantsd to exit when test --debug is used #10051

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signal handling fixes #10758

Signal handling fixes #10758

gshuflin commented Sep 11, 2020 •

edited

stuhood left a comment

stuhood Sep 11, 2020

stuhood Sep 11, 2020

coveralls commented Sep 14, 2020 •

edited

stuhood left a comment

stuhood Sep 14, 2020

gshuflin Sep 14, 2020

stuhood Sep 14, 2020

gshuflin Sep 14, 2020

stuhood Sep 14, 2020

Eric-Arellano commented Sep 15, 2020

		ExceptionSink.toggle_ignoring_sigint(True)
		return self._scheduler.run_local_interactive_process(request)

Signal handling fixes #10758

Signal handling fixes #10758

Conversation

gshuflin commented Sep 11, 2020 • edited

Problem

Solution

Result

stuhood left a comment

Choose a reason for hiding this comment

stuhood Sep 11, 2020

Choose a reason for hiding this comment

stuhood Sep 11, 2020

Choose a reason for hiding this comment

coveralls commented Sep 14, 2020 • edited

stuhood left a comment

Choose a reason for hiding this comment

stuhood Sep 14, 2020

Choose a reason for hiding this comment

gshuflin Sep 14, 2020

Choose a reason for hiding this comment

stuhood Sep 14, 2020

Choose a reason for hiding this comment

gshuflin Sep 14, 2020

Choose a reason for hiding this comment

stuhood Sep 14, 2020

Choose a reason for hiding this comment

Eric-Arellano commented Sep 15, 2020

gshuflin commented Sep 11, 2020 •

edited

coveralls commented Sep 14, 2020 •

edited