https://pantsbuild.org/ logo
#development
Title
# development
h

hundreds-breakfast-49010

09/10/2020, 9:56 PM
but other goals seem not to. e.g.
./pants --no-pantsd fmt src/python/pants::
actually what I think might be going on is that, in the
--no-pantsd
case, a ctrl-c is still getting sent to one of the processes actively being run somehow. and the linters/formatters are handling this better than pytest
but if I run for instance
./pants --no-pantsd lint src/python/pants/engine::
and hit ctrl-c at the right time, I'll see a (say)
black
python traceback message in the console
w

witty-crayon-22786

09/10/2020, 10:14 PM
this is what we discussed yesterday i think. SIGINT will be ignored while we’re in pure rust code until we hit some python code
👍 1
and ctrl+c is a SIGINT
h

hundreds-breakfast-49010

09/10/2020, 10:15 PM
that makes sense
w

witty-crayon-22786

09/10/2020, 10:15 PM
so if there isn’t a thread that is actually touching python code to watch for the signal, it won’t be received
the dynamic-ui should mean that we are touching python code though, in general
…oh, wait. nope. because we now use only the workunit strings, which don’t touch python anymore. interesting.
h

hundreds-breakfast-49010

09/10/2020, 10:17 PM
yeah, I was about to say I don't think we're regularly touching python code
in dynamic-ui anymore
w

witty-crayon-22786

09/10/2020, 10:17 PM
so yea… i think what you could maybe do there is literally intentionally poke a “have i been killed” noop python method in the
execute
loop
h

hundreds-breakfast-49010

09/10/2020, 10:17 PM
which is possibly when this broke?
I think that woudl also result in broken ctrl-c handling in the -no-dynamic-ui case
but I haven't tested that yet
./pants --no-pantsd --no-dynamic-ui test src/python/pants/engine::
doesn't quit immediately on ctrl-c but does after a couple of tries
w

witty-crayon-22786

09/10/2020, 10:19 PM
i expect that that has to do with “eventually” touching some python
h

hundreds-breakfast-49010

09/10/2020, 10:19 PM
ideally we wouldn't have to be touching python to handle a ctrl-c
w

witty-crayon-22786

09/10/2020, 10:20 PM
maybe. but i think we only want to have signal handling implemented in one place
so we’d want to either have it in rust, or in python. right now it’s in python.
h

hundreds-breakfast-49010

09/10/2020, 10:20 PM
we sort of have two mechanisms for doing so right now - the stuff in
SignalHandler
in
exception_sink.py
, which I'm actively messing with now to fix https://github.com/pantsbuild/pants/issues/10051
w

witty-crayon-22786

09/10/2020, 10:20 PM
right. that’s the only one i’m aware of.
h

hundreds-breakfast-49010

09/10/2020, 10:21 PM
and there's also an
except KeyboardInterrupt
block in
pants_exe.py
that is getting exercised at least sometimes
I think the default python behavior is that the python interpreter process's runtime catches SIGINT and turns that into a
KeyboardInterrupt
exception. but we're already overriding that
but sometimes re-raising a
KeyboardInterrupt
ourselves
w

witty-crayon-22786

09/10/2020, 10:32 PM
mm.
are you sure that the ExceptionSink is actually “capturing” the signal, as opposed to just acting on it before it is raised as a KeyboardInterrupt?
regardless, i think that answering that question is maybe less important than figuring out whether https://pantsbuild.slack.com/archives/C0D7TNJHL/p1599776230071800?thread_ts=1599774968.069500&cid=C0D7TNJHL results in the
execute
loop observing the error.
h

hundreds-breakfast-49010

09/10/2020, 10:43 PM
@witty-crayon-22786 I mean I think it's capturing the signal, and then sometimes raising it as a
KeyboardInterrupt
, which that handler in
pants_exe.py
catches
unless there's somewhere else raising a keyboardinterrupt, but I think that's the relevant flow of code
Copy code
A long-running calculation implemented purely in C (such as regular expression matching on a large body of text) may run uninterrupted for an arbitrary amount of time, regardless of any signals received. The Python signal handlers will be called when the calculation finishes.
…there is a lot of good info in the opening of that page.
but i suspect that that paragraph is why we should be poking something from rust to see whether we should exit.
h

hundreds-breakfast-49010

09/10/2020, 10:47 PM
I wonder if we do want (for now) to add explicit signal handling in Rust, that for now triggers a python
KeyboardInterrupt
, and eventually get rid of the python signal handling
a lot of which exists to pass signals to pantsd, which you're interested in getting rid of in any case
w

witty-crayon-22786

09/10/2020, 10:48 PM
i don’t think so.
because the implementation of signal handling in rust would be approximately the same thing: set a flag somewhere, and then check it from the execute loop
h

hundreds-breakfast-49010

09/10/2020, 10:48 PM
when you say "execute loop" what specific area of code do you mean?
w

witty-crayon-22786

09/10/2020, 10:49 PM
the method
Scheduler::execute
h

hundreds-breakfast-49010

09/10/2020, 10:50 PM
so then we'd still want to install explicit signal handling in rust, have it set that flag, have
Scheduler:;execute
respond to that flag, and eventually get rid of python
SignalHandler
w

witty-crayon-22786

09/10/2020, 10:50 PM
no
i don’t think so.
i’m saying that regardless of where signal handlers are installed, you’d need to check a condition in that loop
so my suggestion was to just “touch” the python code to see whether it had been killed by a signal
h

hundreds-breakfast-49010

09/10/2020, 10:52 PM
I think that will work in the --dyanmic-ui case, but there isn't a good way (that I can immediately see anyway) to do so in the --no-dynamic-ui case
although we expect the former case to be more common, so maybe that's okay
w

witty-crayon-22786

09/10/2020, 10:52 PM
it will work without the dynamic ui as well.
the execute loop runs regardless of whether the UI is being used.
but this is relevant too:
Copy code
Python signal handlers are always executed in the main Python thread, even if the signal was received in another thread. This means that signals can't be used as a means of inter-thread communication. You can use the synchronization primitives from the threading module instead.
i think that that means that the python method that we call to see whether we’ve been killed with SIGINT should check a thread-safe global.
so maybe i was wrong about it being better to put it on the python side. but unclear. i think that if you installed signal handlers from rust, you might break python’s VM signal handling though… unclear.
h

hundreds-breakfast-49010

09/10/2020, 10:57 PM
I thought that the point of that python method would just be to return control back to python, so that if SIGINT had been sent, it would be handled by the main python thread at that time, rather than waiting until the engine would return control to python normally
i..e we don't have to do anything at all in that python method
w

witty-crayon-22786

09/10/2020, 10:58 PM
right… that might be the case. but it’s not clear whether the caller of
Scheduler::execute
is the main thread.
maybe it is in all cases.
h

hundreds-breakfast-49010

09/10/2020, 10:59 PM
it's okay if the caller of
Scheduler::execute
isn't the main thread, right? "Python signal handlers are always executed in the main Python thread, even if the signal was received in another thread."
so in this case, the SIGINT handler is the code set up by
SignalHandler
, specifically https://github.com/pantsbuild/pants/blob/master/src/python/pants/base/exception_sink.py#L81
w

witty-crayon-22786

09/10/2020, 11:00 PM
what i think that that means though is that
Scheduler::execute
might not see the keyboard exeception.
h

hundreds-breakfast-49010

09/10/2020, 11:00 PM
which (unless we've swapped it out with
PailgunClientSignalHandler
in the pantsd case) just raises
KeyboardInterrupt
w

witty-crayon-22786

09/10/2020, 11:01 PM
anyway, feels worth experimenting with.
h

hundreds-breakfast-49010

09/10/2020, 11:01 PM
yeah, for sure. I'll give it a shot and see what happens
w

witty-crayon-22786

09/10/2020, 11:01 PM
if you can literally call a noop python method in
Scheduler::execute
and always have it die in practice, then that’s great. if you have to add a global condition variable, that’s still not bad.
👍 1
h

hundreds-breakfast-49010

09/11/2020, 12:26 AM
I still need to clean this up a bit, but it looks like https://github.com/pantsbuild/pants/pull/10758 (which has that explicit check) seems to get snappy ctrl-c in the --no-pantsd case, thanks for the suggestion stu
🎉 1
w

witty-crayon-22786

09/11/2020, 12:26 AM
Cool beans! I'll take a look tomorrow.
h

hundreds-breakfast-49010

09/11/2020, 12:27 AM
although I'm concerned about the fact that we're now calling a function that grabs the python GIL in the scheduler.rs loop, I'm not sure that won't introduce deadlocks
w

witty-crayon-22786

09/11/2020, 12:27 AM
There are no other locks acquired there, so it shouldn't.
👍 1