https://pantsbuild.org/ logo
Join Slack
Powered by
but other goals seem not to. e.g. `./pants --no-pa...
# development
h
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
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
that makes sense
w
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
yeah, I was about to say I don't think we're regularly touching python code
in dynamic-ui anymore
w
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
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
i expect that that has to do with “eventually” touching some python
h
ideally we wouldn't have to be touching python to handle a ctrl-c
w
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
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
right. that’s the only one i’m aware of.
h
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
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
@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
w
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
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
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
when you say "execute loop" what specific area of code do you mean?
w
the method 
Scheduler::execute
h
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
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
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
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
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
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
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
what i think that that means though is that 
Scheduler::execute
might not see the keyboard exeception.
h
which (unless we've swapped it out with 
PailgunClientSignalHandler
in the pantsd case) just raises 
KeyboardInterrupt
w
anyway, feels worth experimenting with.
h
yeah, for sure. I'll give it a shot and see what happens
w
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
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
Cool beans! I'll take a look tomorrow.
h
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
There are no other locks acquired there, so it shouldn't.
👍 1
2 Views
Open in Slack
PreviousNext
Powered by