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

hundreds-breakfast-49010

10/12/2019, 12:02 AM
is there a problem with handling
KeyboardInterrupt
within a console_rule?
a

aloof-angle-91616

10/15/2019, 5:57 PM
yes -- does this not work?
h

hundreds-breakfast-49010

10/15/2019, 8:20 PM
yeah, I'm running into this developing a
@console_rule
for running subprocesses
a

aloof-angle-91616

10/15/2019, 8:21 PM
ok
do you have a branch?
i have time to look at it now
a

aloof-angle-91616

10/15/2019, 8:21 PM
thanks!
will timebox to 30 minutes
h

hundreds-breakfast-49010

10/15/2019, 8:22 PM
right now this is running a dummy program I wrote called
test.py
, but that could be any long-running program
a

aloof-angle-91616

10/15/2019, 8:22 PM
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

hundreds-breakfast-49010

10/15/2019, 8:22 PM
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

aloof-angle-91616

10/15/2019, 8:22 PM
yeah that's because the error handling is bad somewhere
h

hundreds-breakfast-49010

10/15/2019, 8:23 PM
yeah, and that's what I'm ultimately trying to figure out how tohandle
a

aloof-angle-91616

10/15/2019, 8:23 PM
the FFI is supposed to return null i think when there are errors and the errors are stored somewhere else
h

hundreds-breakfast-49010

10/15/2019, 8:23 PM
since we'd like to be able to catch a keyboard interrupt in a console_rule
a

aloof-angle-91616

10/15/2019, 8:23 PM
ok, take a look at the above PR first then
for context
yeah
and i'll check out your branch
h

hundreds-breakfast-49010

10/15/2019, 8:25 PM
I don't think my branch does anything differently than master with regard to signal-handling, except try to make use of it
a

aloof-angle-91616

10/15/2019, 8:25 PM
yes
oh well
there's a whole bunch of work around signal handling in
ExceptionSink
h

hundreds-breakfast-49010

10/15/2019, 8:26 PM
yeah I'm trying to understand how all of that works at the moment
a

aloof-angle-91616

10/15/2019, 8:26 PM
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

hundreds-breakfast-49010

10/15/2019, 8:26 PM
my
test.py
is doing this:
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
a

aloof-angle-91616

10/15/2019, 8:27 PM
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

hundreds-breakfast-49010

10/15/2019, 8:28 PM
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

aloof-angle-91616

10/15/2019, 8:28 PM
yes
thank you greg that's exactly it
h

hundreds-breakfast-49010

10/15/2019, 8:29 PM
also something is printing out the string "KeyboardInterrupt" and I'm not sure what
I guess that'sthe default python behavior?
a

aloof-angle-91616

10/15/2019, 8:29 PM
that's what
str(KeyboardInterrupt)
returns
like
or no
str(KeyboardInterrupt())
it's the default message
super mysterious
h

hundreds-breakfast-49010

10/15/2019, 8:29 PM
wait n/m that's coming from my test program not pants
a

aloof-angle-91616

10/15/2019, 8:29 PM
R is so much better at this
h

hundreds-breakfast-49010

10/15/2019, 8:30 PM
just becuase my test program is also a python program
a

aloof-angle-91616

10/15/2019, 8:30 PM
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

hundreds-breakfast-49010

10/15/2019, 8:39 PM
oh you think you know where in the rust code is the right place to handle a signal is?
a

aloof-angle-91616

10/15/2019, 8:40 PM
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

hundreds-breakfast-49010

10/15/2019, 8:42 PM
we're currently only using that in
remote_pants_runner
it looks like
a

aloof-angle-91616

10/15/2019, 8:42 PM
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

hundreds-breakfast-49010

10/15/2019, 9:13 PM
solid
I was up and away from my desk talking through something else with one of my coworkers, catching up now
a

aloof-angle-91616

10/15/2019, 9:14 PM
which coworkers
and no worries, i was able to figure it out in that time
h

hundreds-breakfast-49010

10/15/2019, 9:14 PM
alex had some questions related to some of the fs.py stuff I had worked on a few weeks ago
šŸ‘Œ 1
a

aloof-angle-91616

10/15/2019, 9:14 PM
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

hundreds-breakfast-49010

10/15/2019, 9:15 PM
do you have a commit at hand?
a

aloof-angle-91616

10/15/2019, 9:15 PM
i could
w

witty-crayon-22786

10/15/2019, 9:16 PM
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)
w

witty-crayon-22786

10/15/2019, 9:17 PM
the intrinsic that actually "does things" should likely be implementing it
a

aloof-angle-91616

10/15/2019, 9:17 PM
yes
h

hundreds-breakfast-49010

10/15/2019, 9:18 PM
@witty-crayon-22786 this originally came up becuase I thought I could implement the subprocess-spawning faculty we need purely in python
a

aloof-angle-91616

10/15/2019, 9:18 PM
oh oops i need to calculate the runtime errors in each iteration of the loop
h

hundreds-breakfast-49010

10/15/2019, 9:18 PM
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

aloof-angle-91616

10/15/2019, 9:20 PM
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

hundreds-breakfast-49010

10/15/2019, 9:21 PM
so this is a separate self-contained thing, that will make it possible for rules to capture a keyboard interrupt?
a

aloof-angle-91616

10/15/2019, 9:22 PM
yes i believe so
h

hundreds-breakfast-49010

10/15/2019, 9:22 PM
I mean, regarldess, getting rid of the awkward null c pointer error message would be good
awesome
a

aloof-angle-91616

10/15/2019, 9:22 PM
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

hundreds-breakfast-49010

10/15/2019, 9:24 PM
great, I'll take a look at the PR when it's pushed
w

witty-crayon-22786

10/15/2019, 9:24 PM
h

hundreds-breakfast-49010

10/15/2019, 9:31 PM
@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

aloof-angle-91616

10/15/2019, 9:33 PM
i'm close -- have been compiling rust
w

witty-crayon-22786

10/15/2019, 9:34 PM
@hundreds-breakfast-49010: as mentioned above: rules should probably not be able to implement exception handling
ā˜ļø 1
a

aloof-angle-91616

10/15/2019, 9:34 PM
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

hundreds-breakfast-49010

10/15/2019, 9:35 PM
right now (without @aloof-angle-91616ā€™s change) is it not possible to use a try/except block in a rule at all?
w

witty-crayon-22786

10/15/2019, 9:36 PM
anyway. this has been discussed quite a bit. it is what it is.
a

aloof-angle-91616

10/15/2019, 9:36 PM
it is, but
KeyboardInterrupt
isn't handled correctly
that's this fix
w

witty-crayon-22786

10/15/2019, 9:37 PM
@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

hundreds-breakfast-49010

10/15/2019, 9:38 PM
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

witty-crayon-22786

10/15/2019, 9:44 PM
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

hundreds-breakfast-49010

10/15/2019, 9:50 PM
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

aloof-angle-91616

10/15/2019, 9:50 PM
i am writing the PR description now
and yes, i think so
h

hundreds-breakfast-49010

10/15/2019, 9:52 PM
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

witty-crayon-22786

10/15/2019, 9:53 PM
intrinsics should do it.
a

aloof-angle-91616

10/15/2019, 10:00 PM
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

hundreds-breakfast-49010

10/15/2019, 10:14 PM
kk
a

aloof-angle-91616

10/15/2019, 10:28 PM
i have asked for review and described why testing is hard
h

hundreds-breakfast-49010

10/15/2019, 11:57 PM
@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

witty-crayon-22786

10/16/2019, 12:29 AM
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

hundreds-breakfast-49010

10/16/2019, 12:30 AM
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

witty-crayon-22786

10/16/2019, 12:32 AM
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

hundreds-breakfast-49010

10/16/2019, 12:32 AM
right, and that's what I'm trying to implement
w

witty-crayon-22786

10/16/2019, 12:33 AM
Ok. Those would be implemented as an intrinsic.
And at that point the intrinsic would handle interrupts
h

hundreds-breakfast-49010

10/16/2019, 12:35 AM
so that means that that intrinsic can't yield back to python code until the process is done
w

witty-crayon-22786

10/16/2019, 12:36 AM
Correct. And it wouldn't need to, afaik
h

hundreds-breakfast-49010

10/16/2019, 12:36 AM
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

witty-crayon-22786

10/16/2019, 12:37 AM
Well, having the intrinsic capture that would require changes in the general area that Danny was editing... would require some cooperation there.
h

hundreds-breakfast-49010

10/16/2019, 12:39 AM
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

witty-crayon-22786

10/16/2019, 12:41 AM
Re: "block on" it depends. There are lots of APIs to use there.
h

hundreds-breakfast-49010

10/16/2019, 12:41 AM
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

witty-crayon-22786

10/16/2019, 12:41 AM
You could poll with an Xms loop, and watch for a signal that you had been killed, etc
h

hundreds-breakfast-49010

10/16/2019, 12:42 AM
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

witty-crayon-22786

10/16/2019, 12:42 AM
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

hundreds-breakfast-49010

10/16/2019, 12:43 AM
which method in scheduler.rs?
execute
?
w

witty-crayon-22786

10/16/2019, 12:43 AM
Yea
h

hundreds-breakfast-49010

10/16/2019, 12:44 AM
right, that's where the v2 ui code that polls every 100 ms lives
I'm also looking at that at the moment
w

witty-crayon-22786

10/16/2019, 12:44 AM
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

hundreds-breakfast-49010

10/16/2019, 12:46 AM
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

witty-crayon-22786

10/16/2019, 12:51 AM
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

hundreds-breakfast-49010

10/16/2019, 12:52 AM
how else can pants possibly handle a ctrl-c?
w

witty-crayon-22786

10/16/2019, 12:52 AM
In the intrinsic.
h

hundreds-breakfast-49010

10/16/2019, 12:53 AM
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

witty-crayon-22786

10/16/2019, 12:53 AM
Yes. The rule wouldn't even get involved
Well. Correction: it would be a FalliableExecuteProcessRequest, so the rule could inspect the result
h

hundreds-breakfast-49010

10/16/2019, 7:35 PM
what infrastructure do we need to make sure that this rule is only yield-able from within a
console_rule
?
w

witty-crayon-22786

10/16/2019, 7:58 PM
i don't think that part is a blocker. you opened a ticket for it
h

hundreds-breakfast-49010

10/16/2019, 7:58 PM
it's not a blocker necessarily, but it does raise the question of whether an intrinsic is the right way to do this
w

witty-crayon-22786

10/16/2019, 7:59 PM
but i also commented on a PR about the non-cacheability of rules, and that discussion is related.
h

hundreds-breakfast-49010

10/16/2019, 7:59 PM
ah, okay, there's more comments on that issue now, will look