https://pantsbuild.org/ logo
Join Slack
Powered by
is there a problem with handling `KeyboardInterrup...
# development
h
is there a problem with handling 
KeyboardInterrupt
within a console_rule?
a
yes -- does this not work?
h
yeah, I'm running into this developing a 
@console_rule
for running subprocesses
a
ok
do you have a branch?
i have time to look at it now
h
a
thanks!
will timebox to 30 minutes
h
right now this is running a dummy program I wrote called 
test.py
, but that could be any long-running program
a
got it
so one thing that i learned
is that 
KeyboardInterrupt
is not a subclass of 
Exception
, but rather of 
BaseException
there's a PR, here:
h
if you do 
./pants v2-run
on that branch, and hit ctrl-c, the exception you get is ultimately from a FFI null pointer
a
yeah that's because the error handling is bad somewhere
h
yeah, and that's what I'm ultimately trying to figure out how tohandle
a
the FFI is supposed to return null i think when there are errors and the errors are stored somewhere else
h
since we'd like to be able to catch a keyboard interrupt in a console_rule
a
ok, take a look at the above PR first then
for context
yeah
and i'll check out your branch
h
I don't think my branch does anything differently than master with regard to signal-handling, except try to make use of it
a
yes
oh well
there's a whole bunch of work around signal handling in 
ExceptionSink
h
yeah I'm trying to understand how all of that works at the moment
a
which also provides convenience methods so you don't have to do the work
yes
what command line should i run
oop
sorry
got it
h
my 
test.py
is doing this:
Untitled
Untitled
super simple, just a way of getting a program that runs long enough for me to try to ctrl-c it
and the error message I get
is
Untitled
Untitled
a
yeah
so basically what i am pretty sure about but literally should just check the 
cffi
docs for is that the NULL pointer happens when there's an exception and it's not handled. what i am sure about is that we have already figured this out somewhere. just gotta find it brb
h
and I think this makes sense because we're in a console_rule when the keyboard interrupt happens, so rust FFI functions are above it in the call stack
🔥 1
💯 1
ok
a
yes
thank you greg that's exactly it
h
also something is printing out the string "KeyboardInterrupt" and I'm not sure what
I guess that'sthe default python behavior?
a
that's what 
str(KeyboardInterrupt)
returns
like
or no
str(KeyboardInterrupt())
it's the default message
super mysterious
h
wait n/m that's coming from my test program not pants
a
R is so much better at this
h
just becuase my test program is also a python program
a
yes
so yeah
mysterious 
KeyboardInterrupt
stringification just means nobody provided a message
super great, easily reproed
i think i got it and it's not bad, will push a branch in a few
h
oh you think you know where in the rust code is the right place to handle a signal is?
a
i think it's just not clear how to override a signal handler for dastardly deeds and use 
ExceptionSink.trapped_signals()
with a subclass of 
SignalHandler
h
we're currently only using that in 
remote_pants_runner
it looks like
a
yes
signal handling is extremely hard
(well, we're making it easier!)
oooooo
ok
so
i did the signal handling correctly i think, or i thought, but now the gods are mad at me because python `@rule`s don't run on the main thread
Copy code
Computing Select((<pants.engine.console.Console object at 0x1b9137e5450>+<pants.engine.interactive_runner.InteractiveRunner object at 0x1b9137e5550>+Specs(dependencies=(), matcher=SpecsMatcher(tags=(), exclude_patterns=()))), Run)
  Computing Task(run(), (<pants.engine.console.Console object at 0x1b9137e5450>+<pants.engine.interactive_runner.InteractiveRunner object at 0x1b9137e5550>+Specs(dependencies=(), matcher=SpecsMatcher(tags=(), exclude_patterns=()))), Run, false)
    Throw(signal only works in main thread)
      Traceback (most recent call last):
        File "/Users/dmcclanahan/tools/pants-v6/src/python/pants/engine/native.py", line 463, in extern_generator_send
          res = c.from_value(func[0]).send(c.from_value(arg[0]))
        File "/Users/dmcclanahan/tools/pants-v6/src/python/pants/rules/core/run.py", line 38, in run
          with ExceptionSink.trapped_signals(SubprocessKillingHandler(res.popen)):
        File "/usr/local/Cellar/python/3.7.4_1/Frameworks/Python.framework/Versions/3.7/lib/python3.7/contextlib.py", line 112, in __enter__
          return next(self.gen)
        File "/Users/dmcclanahan/tools/pants-v6/src/python/pants/base/exception_sink.py", line 376, in trapped_signals
          previous_signal_handler = cls.reset_signal_handler(new_signal_handler)
        File "/Users/dmcclanahan/tools/pants-v6/src/python/pants/base/exception_sink.py", line 355, in reset_signal_handler
          signal.signal(signum, handler)
        File "/usr/local/Cellar/python/3.7.4_1/Frameworks/Python.framework/Versions/3.7/lib/python3.7/signal.py", line 47, in signal
          handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
      ValueError: signal only works in main thread
so then i'm like ok yeah this is for when you want to change signal handling for a whole process, like in the RemotePantsRunner
so let's absolutely just step back and try to catch the initial KeyboardInterrupt
i hadn't realized that 
ExceptionSink
might not be at all usable in `@rule`s because of assumptions about being on the main thread. it's fine for now, but a good note for the future imho
(thanks greg!)
diving into 
native.py
again now
YES ok i actually got it
just need another 
if
in 
scheduler.py
lit
h
solid
I was up and away from my desk talking through something else with one of my coworkers, catching up now
a
which coworkers
and no worries, i was able to figure it out in that time
h
alex had some questions related to some of the fs.py stuff I had worked on a few weeks ago
👌 1
a
ok it's not at all working yet but it definitely is acting super weirdly which means i'm breaking the right things
super cool
h
do you have a commit at hand?
a
i could
w
rules implementing exception handling would be pretty odd i think. they're coroutines, for one thing (meaning that they aren't actually running when something fails)
a
w
the intrinsic that actually "does things" should likely be implementing it
a
yes
h
@witty-crayon-22786 this originally came up becuase I thought I could implement the subprocess-spawning faculty we need purely in python
a
oh oops i need to calculate the runtime errors in each iteration of the loop
h
so there's no engine intrinsic
unless this signal-handling thing requires that subprocess-spawning be done as an intrinsic rather than in python
a
DID IT
this was already handled
just wasn't hooked up right for console rules
i think
will push the actual fix
Copy code
Exception message: 1 Exception encountered:
Computing Select((<pants.engine.console.Console object at 0x1b917041ad0>+<pants.engine.interactive_runner.InteractiveRunner object at 0x1b917041410>+Specs(dependencies=(), matcher=SpecsMatcher(tags=(), exclude_patterns=()))), Run)
  Value(Run(exit_code=0))
when using ctrl-c
h
so this is a separate self-contained thing, that will make it possible for rules to capture a keyboard interrupt?
a
yes i believe so
h
I mean, regarldess, getting rid of the awkward null c pointer error message would be good
awesome
a
right???
that's been there for so long
it would make me incredibly happy
wait yeah absolutely
your 
test.py
totally works catching the exception
it's cool
pushing in a few mins
h
great, I'll take a look at the PR when it's pushed
w
h
@witty-crayon-22786 I'm trying to get a draft PR ready so we have some actual code to look at. I'm not married to implementing subprocress-spawning in python, but right now I can't think of a reason why implementing it on the rust side would be significantly different
but either way I think it's easier to think about with a draft implementation at hand
a
i'm close -- have been compiling rust
w
@hundreds-breakfast-49010: as mentioned above: rules should probably not be able to implement exception handling
☝️ 1
a
oh hm
@witty-crayon-22786 i think this change is a fix regardless (because of a bug that cropped up here), but yes, i 100% agree with your message
h
right now (without @aloof-angle-91616’s change) is it not possible to use a try/except block in a rule at all?
w
anyway. this has been discussed quite a bit. it is what it is.
a
it is, but 
KeyboardInterrupt
isn't handled correctly
that's this fix
w
@hundreds-breakfast-49010 right now an exception will never be raised in a rule via 
yield Get
, which rules out "most" cases of exceptions
and makes handling them rare.
h
what happens right now if you, say, write a rule that happens to divide by zero?
I just asumed that you could use normal python try/except blocks as part of the implementation of a rule, although I don't think I've tried to
or tried to see what happnes if you let an exception deliberately bubble out of a rule
ah ok that just gets caught by the existing exception handling functionality
w
the rule itself will raise an exception, but no other rule would be able to catch it
so a rule could catch its own exceptions.
h
so in this case the specific exception is a KeybordInterrupt
which doesn't come from the rule itself
it sounds like @aloof-angle-91616’s change makes it possible for a rule to have a 
except KeyboardInterrupt:
block, but that we might not want to allow that in a rule anyway?
👍 1
a
i am writing the PR description now
and yes, i think so
h
so, how could a run 
@console_rule
work in principle, without being able to ignore a KeyboardInterrupt that only should get sent to a subprocess?
I'm actually not sure what unix signal that is
w
intrinsics should do it.
a
put up a PR fixing some cffi errors we weren't capturing: https://github.com/pantsbuild/pants/pull/8478
not quite yet ready for review
i need to shower and do something else for a bit but will add a test later today
h
kk
a
i have asked for review and described why testing is hard
h
@witty-crayon-22786 I don't see how it's possible to implement subprocesses as an intrinsic while also maintaining the invariant that rules don't have side effects
or yielding back to the python code while the subprocess is running
in fact, unless we deliberately want to block on an engine intrinsic that runs the subprocess, which I think we don't want to do for this case, then a python console_rule has to be able a KeyboardInterrupt
w
Rules suspend while they are waiting for something that they have yielded for (coroutines do, in general)
So no, you don't block. But the idea behind yielding to a intrinsic is that the rule suspends while it is running.
h
right, so is that the behavior we want for executing a subprocess?
I was under the impression that we don't want this
i.e. the 
run
console_rule should not yield to the engine and wait for whatever user executable is running to finish before control comes back to the console_rule
w
I though that that is exactly what we had described as `foreground`/`exclusivity`/"the other type that you were describing on the doc that I pointed out was parallel"
h
right, and that's what I'm trying to implement
w
Ok. Those would be implemented as an intrinsic.
And at that point the intrinsic would handle interrupts
h
so that means that that intrinsic can't yield back to python code until the process is done
w
Correct. And it wouldn't need to, afaik
h
and a ctrl-c will still get captured by python, which we want to suppress while the subprocess is running
unless I'm missing something about how signal handling works
which I might be
w
Well, having the intrinsic capture that would require changes in the general area that Danny was editing... would require some cooperation there.
h
yeah figuring out the right way to explicitly handle SIGINT while pants is running a subprocess is exactly what I'm trying to do right now
so, ok, if I implement this as an intrinsic, then the 
run
console_rule will yield to the engine with some type, the engine will receive that type, spawn a subprocess, and then block on that subprocess' completion
and while it's doing that it will also listen for SIGINT and make sure that it causes that signal to be sent to the subprocess without exiting itself
w
Re: "block on" it depends. There are lots of APIs to use there.
h
my original thought was that the rule would yield back to python immediately with some type representing a handle to a currently-running process
w
You could poll with an Xms loop, and watch for a signal that you had been killed, etc
h
and then I realized that this is basically what 
Popen
was, which is why I thought I could do it with the python subprocess module without invoking the engine
and then the console rule would do that polling (in python), or maybe call .wait() on the 
Popen
, whatever made sense
w
We do a thing sort of like this at the very top of the engine in scheduler.rs, but we're not sending any signals there
h
which method in scheduler.rs?
execute
?
w
Yea
h
right, that's where the v2 ui code that polls every 100 ms lives
I'm also looking at that at the moment
w
So, if in ExceptionSink we set/sent a signal that interrupted that loop, we could exit cleanly there
That would not kill child processes though... that's related, but a different loop.
And yea, this is complicated.
h
yeah like I said I was oriignally just trying to write something that solved the 
run
problem, so we could all have some actual code in front of us to look at
in this case, the run rule will know what the child process that it cares about it, becuase it will have some kind of reference ot it
so as long as that rule can respond to a ctrl-c itself, it can kill that subprocess or print error messages from it or do whatever management it needs to
and maybe that means that ExceptionSink needs to (now) handle SIGINT generally, just in case it's being sent while the run console_rule is running
w
So... again, I feel that that is very opposed to "rules not implementing exception handling", so I'm not really in favor of that.
I understand what you are saying, but... yea
h
how else can pants possibly handle a ctrl-c?
w
In the intrinsic.
h
so when the intrinsic starts, it would set up a SIGINT signal handler, spawn a subprocess, and then when that subprocess had existed it would remove that handler
and yield back to the run console rule?
w
Yes. The rule wouldn't even get involved
Well. Correction: it would be a FalliableExecuteProcessRequest, so the rule could inspect the result
h
what infrastructure do we need to make sure that this rule is only yield-able from within a 
console_rule
?
w
i don't think that part is a blocker. you opened a ticket for it
h
it's not a blocker necessarily, but it does raise the question of whether an intrinsic is the right way to do this
w
but i also commented on a PR about the non-cacheability of rules, and that discussion is related.
h
ah, okay, there's more comments on that issue now, will look
Open in Slack
PreviousNext
Powered by