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

hundreds-breakfast-49010

09/18/2020, 8:04 PM
I've been looking at cases where ctrl-c still doesn't promptly exit pants, and one of the cases where I've seen that is with pantsd tests
specifically pantsd tests run with the
--debug
flag to foreground them; e.g.
./pants test --debug tests/python/pants_test/pantsd/pantsd_integration_test.py
it's very easy for a ctrl-c to quit this test but leave the test instance of pantsd running in the background and still trying to output things to the terminal
this might be fixable by changing how our own pantsd test harness works; but we also might expect pants users to set up tests that test daemonized processes in a similar way to what we're doing
a

aloof-angle-91616

09/18/2020, 8:13 PM
i have been saying we need to work on the pantsd test harness for a while and i tried very hard and did not get anywhere over a year ago and i'd love to try again
h

hundreds-breakfast-49010

09/18/2020, 8:15 PM
we could definitely do work on the pantsd test harness; but also this seems like a case that pants should be able to handle
a

aloof-angle-91616

09/18/2020, 8:15 PM
yes agreed
h

hundreds-breakfast-49010

09/18/2020, 8:16 PM
I'm actually seeing now that even in non-pantsd tests,
--debug
seems to cause ctrl-c to not actually quit tests processes, which I think might be because when the main instance of pantsd that is running tests exits, it doesn't necessarily kill the tests
a

aloof-angle-91616

09/18/2020, 8:17 PM
ah, that's interesting
that's kind of weird. i thought you had to do something special to make sure child processes don't also die when the parent dies?
h

hundreds-breakfast-49010

09/18/2020, 8:18 PM
I'm honestly not sure
a

aloof-angle-91616

09/18/2020, 8:18 PM
i get the problem now though thanks
w

witty-crayon-22786

09/18/2020, 9:15 PM
the nailgun heartbeating stuff that i added a while back is supposed to be a step in the direction of the server canceling runs when the client goes away
nails
now gives a guarantee that the client input only stays open as long as the client is connected and sending heartbeats. but we don’t do anything with that information in pants’ nailgun server yet.
@hundreds-breakfast-49010: if you’re interested in taking that on, i can add more details to https://github.com/pantsbuild/pants/issues/10057
a

aloof-angle-91616

09/18/2020, 9:22 PM
i also might have time this weekend
h

hundreds-breakfast-49010

09/18/2020, 9:23 PM
yeah ultimately what I'm trying to do right now is get https://github.com/pantsbuild/pants/issues/10057 into a state where we can consider it closed
w

witty-crayon-22786

09/18/2020, 9:23 PM
cool beans. i’ll add the details, and you two can decide who works on what when
h

hundreds-breakfast-49010

09/18/2020, 9:23 PM
so if that requires implementing the heartbeat stuff, then it makes sense to work on it
anyway what I'm seeing locally is that pantsd seems to be successfully killed, but then there's still a few
python3.6
processes around
/usr/bin/python3.6 ./test_runner.pex --no-header src/python/pants/engine/internals/scheduler_integration_test.py
and
/usr/bin/python3.6 -m pants --no-pantsrc --pants-workdir=/tmp/.tmpjyLQzy/.pants.d/tmp/tmp6g359434.pants.d --print-exception-stacktrace=True --pantsd --pants-config-files=[] --backend-packages=['pants.backend.python', 'internal_plugins.rules_for_testing'] list-and-die-for-testing examples/src/python/example/hello/greet
(this might vary with the specific test being run)
so it does seem like a process spawned via the
InteractiveProcess
mechanism is sticking around on the system even though
pantsd
itself has quit
w

witty-crayon-22786

09/18/2020, 9:31 PM
would try to repro that in a smaller case. that’s not what i was thinking of above.
h

hundreds-breakfast-49010

09/18/2020, 9:32 PM
yeah I think this specific ctrl-c-related issue might be unrelated
w

witty-crayon-22786

09/18/2020, 9:32 PM
ie, would see if running a “sleep forever” process using
./pants run
, and then ctrl+c’ing it kills the sleep forever.
👍 1
"Children don't automatically die when the parent process is killed"
a

aloof-angle-91616

09/18/2020, 11:42 PM
wow
thank you
that is very surprising for some reason
h

hundreds-breakfast-49010

09/18/2020, 11:42 PM
I'm not sure if this is a claim about how python popen works or if this is a general unix processs thing I was unaware of
a

aloof-angle-91616

09/18/2020, 11:43 PM
yeah exactly
h

hundreds-breakfast-49010

09/18/2020, 11:43 PM
but yeah I also thought that by default processes were tied to their parent, and you had to do something special to make that not be the case
a

aloof-angle-91616

09/18/2020, 11:43 PM
i do know that if you provide the PGRP instead of the PID that it kills the process tree
and i think the PGRP is just negative of the PID
that's weird i am about to post a twitter diff but i will see if that's true in the general case
h

hundreds-breakfast-49010

09/18/2020, 11:49 PM
so a solution to this problem seems like having the pants signal handlers send a signal to any subprocesses spawned by
InteractiveProcess
(and we'd need to keep track of this somewhere)
I don't know if this is the best solution
a

aloof-angle-91616

09/18/2020, 11:51 PM
well to be clear this issue is happening like when pantsd dies right? could we have something happen at pantsd teardown like once it's 100% complete and about to exit, sending SIGKILL to its PGRP?
that would make it exit with an error code but idk if anything is looking at its return code
h

hundreds-breakfast-49010

09/18/2020, 11:58 PM
hm, maybe
yeah thsi only happens when pantsd is running
./pants --no-pantsd test --debug src/python/pants/engine/internals/engine_test.py
for instance doesn't leave any test process running
a

aloof-angle-91616

09/18/2020, 11:59 PM
it would mean we wouldn't have to keep track of anything else to fix the issue is all. seems like there are a lot of places where we have subprocesses (like watchman or the lmdb gc service)
h

hundreds-breakfast-49010

09/18/2020, 11:59 PM
and I'm not sure why that's the case
a

aloof-angle-91616

09/18/2020, 11:59 PM
me neither
h

hundreds-breakfast-49010

09/19/2020, 12:00 AM
if
--debug
is passed, then the subprocess is executed via the rust
Command
mechanism in the engine
oh wait, actually, this might be just the currently-running pytest process capturing the ctrl-c itself
which is what's supposed to happen
👍 1
because I get a different stack trace then
👌 1
a

aloof-angle-91616

09/19/2020, 12:00 AM
ok the main difference with pantsd vs non-pantsd is that ^C is recieved by the
RemotePantsRunner
so i would look to see if we're forwarding the signal correctly there
good analysis
h

hundreds-breakfast-49010

09/19/2020, 12:01 AM
if I run more than one test at a time, I think other processes might still stay alive
yeah
if I do
./pants --no-pantsd test --debug src/python/pants/engine/internals::
then if I hit ctrl-c, whatever process si currently running will die, but then the next one takes over
until I've killed every test that runs
a

aloof-angle-91616

09/19/2020, 12:01 AM
is that the behavior we want?
h

hundreds-breakfast-49010

09/19/2020, 12:02 AM
and now that I think about it I think this was one of the use cases @polite-garden-50641 identified as buggy in a ticket, let me see if I can dig that up
@aloof-angle-91616 I'm not sure. I think we imagined that we'd use
--debug
to debug one test at a time. but nothing right now stops anyone from passing it with more than one test target
curious if you have any thoughts on what the right behavior here should be @witty-crayon-22786
or @hundreds-father-404
h

hundreds-father-404

09/19/2020, 12:04 AM
--debug
is interesting. With
repl
and
run
, we know we’ll only have one interactive process Until recently, it was the same with
--debug
, but we recently loosened the constraints so that you could run as many tests as you want
w

witty-crayon-22786

09/19/2020, 12:05 AM
@hundreds-breakfast-49010: you changed this just recently in https://github.com/pantsbuild/pants/pull/10758, right?
h

hundreds-breakfast-49010

09/19/2020, 12:06 AM
right, that commit was about making pantsd match the behavior of no-pantsd. the right thing was happening in the no-pantsd case - at least if only one target was being run interactively
but if we have more than one target being run interactively, like inthe
--debug
case, I'm not sure what the best thing to do is
w

witty-crayon-22786

09/19/2020, 12:07 AM
both the parent and the child should be killed, probably
whether that requires killing the pgroup, or forwarding the signal, unclear.
h

hundreds-breakfast-49010

09/19/2020, 12:07 AM
so to make that work, I think we would need to have pants go back to capturing the ctrl-c,never letting it get to the interactive process, and then having pants kill the entire process group
h

hundreds-father-404

09/19/2020, 12:08 AM
Counterpoint is that with
repl
, I like the non-pantsd behavior that it results in a
KeyboardInterrupt
being printed from the Repl, which is what you’d get without Pants too. Pants is a light wrapper (The pantsd behavior is different than non-pantsd for
repl
)
h

hundreds-breakfast-49010

09/19/2020, 12:08 AM
and I think these were the considerations we had in mind a while ago when we decided to add the feature that
InteractiveProcess
doesn't let pants itself handle ctrl-c (which was months ago)
h

hundreds-father-404

09/19/2020, 12:09 AM
I’m not sure what makes sense here. Generally, I bias towards “behave how it would behave without Pants” But that’s not a great guide with `--debug`; usually you’d use one single process for all your tests
h

hundreds-breakfast-49010

09/19/2020, 12:09 AM
but at that time, we weren't running pantsd by default
and the bug I fixedin https://github.com/pantsbuild/pants/pull/10758 was basically pantsd doing something different from pants with no daemon
does anyone run the
pytest
binary directly to run tests in a python repo?
I mean typically I would expect that people have some kind of harness around running that directly, whether that's pants or a shell script or Makefile or something
h

hundreds-father-404

09/19/2020, 12:10 AM
does anyone run the pytest binary directly to run tests in a python repo?
Yes, for sure. This is the default flow. Or a light shell script that runs it, yeah But that script is only running Pytest in one subprocess call, usually
a

aloof-angle-91616

09/19/2020, 12:27 AM
when we use
--debug
, are we piping out in our rust code to stdout and stderr? or are we letting it inherit the parent file descriptors? i'm not sure if that's related to why ^C gets sent to the child instead of pants
i will look ^_^
h

hundreds-breakfast-49010

09/19/2020, 12:29 AM
oh hm
this code lives in
<http://interface.rs|interface.rs>
the rust std lib
Command
api has some faculties for doing things to the std* file descriptors that I don't think we're using?
a

aloof-angle-91616

09/19/2020, 12:30 AM
i bet it's stdin
a

aloof-angle-91616

09/19/2020, 12:31 AM
thanks
h

hundreds-breakfast-49010

09/19/2020, 12:31 AM
"Inherit stdin/stdout/stderr for
spawn
or
status
, but create pipes for
output"
so we call
.spawn
later on in the file
but anyway we did originally want Ctrl-C to be sent to an interactive process (and only an interactive process -that's one of the things that makes interactive processes different)
I wonder if maybe the right solution is actually to require that
--debug
only be used with one target
the point of
--debug
is so that you can run a test interactively, so you can debug it
a

aloof-angle-91616

09/19/2020, 12:33 AM
well but i would want to be able to
./pants test --debug ::
though maybe in a small repo
h

hundreds-breakfast-49010

09/19/2020, 12:33 AM
actually right now if you do that, does each test run serially?
a

aloof-angle-91616

09/19/2020, 12:33 AM
will check
h

hundreds-breakfast-49010

09/19/2020, 12:34 AM
it looks like it might be?
a

aloof-angle-91616

09/19/2020, 12:34 AM
blah bootstrapping. will check the code
h

hundreds-breakfast-49010

09/19/2020, 12:34 AM
in a small repo you might actually want the behavior where each test runs serially, and you do have to hit ctrl-c several times to quit each test individually
but that gets old real quick in a repo the size ofpants'
a

aloof-angle-91616

09/19/2020, 12:38 AM
Copy code
for debug_request in debug_requests:
            if debug_request.process is None:
                continue
            debug_result = interactive_runner.run(debug_request.process)
            if debug_result.exit_code != 0:
                exit_code = debug_result.exit_code
so yes they do run serially for
--debug
it looks like we could add an option to the test subsystem like
--debug --fail-fast
which would avoid the multiple ^C (and default it to False maybe)
h

hundreds-breakfast-49010

09/19/2020, 12:42 AM
or maybe that should even be the default behavior. but either way, that's saying that the way pants handles a ctrl-c with interactive processes is right, and it's specifically the behavior of the
test
goal that should change to accomodate it
I think that's reasonable though
a

aloof-angle-91616

09/19/2020, 12:43 AM
yes i think so
it's nice that we don't have to specially handle ^C here in our rust code and can just let it forward
h

hundreds-breakfast-49010

09/19/2020, 12:50 AM
so actually, doing the right thing in this loop, might require the
debug_result
to have a way to distinguish an actual test failure from a ctrl-c
and I'm not sure if there's a way to do that at the
pytest
level
(and of course we shouldn't write code that assumes
pytest
- other test runners exist)
a

aloof-angle-91616

09/19/2020, 12:50 AM
i feel like the
--fail-fast
behavior is a pretty reasonable flag to have
i would be surprised that getting a normal test failure doesn't stop it
h

hundreds-father-404

09/19/2020, 1:21 AM
Yes, it was important to change the restriction that
--debug
only applies to one target. This was crucial when we made the change to the model of targets, where now Pants always runs one-file-at-a-time (unless we batch). Without the change,
./pants test path/to:target
would complain if the target had >1 file
i feel like the --fail-fast behavior is a pretty reasonable flag to have
Possibly. We thought about that when loosening
--debug
, but didn’t add because you’re not expected to use
--debug
for more than when you need a debugger. So we didn’t want to optimize for it;
test
already has a ton of options
a

aloof-angle-91616

09/19/2020, 3:53 AM
"has a ton of options" is confusing to me because i remember the v1 pants options page starts with "...has literally hundreds of configurable options". are we concerned that adding this option will introduce behavior that would be hard to move elsewhere? or does
./pants help
become unwieldy for so many options on one subsystem?
h

hundreds-father-404

09/19/2020, 3:59 AM
or does ./pants help become unwieldy for so many options on one subsystem?
Options fatigue. It gets overwhelming when you have too much choice. It also (often) makes it harder for plugin authors to implement things the more options we add. For example, adding
--test-force
was worth it, but it means that plugin authors now need to be careful to respect the flag
a

aloof-angle-91616

09/19/2020, 3:59 AM
where does the slippery slope end? what process do we use to decide whether this is occurring?
h

hundreds-father-404

09/19/2020, 4:02 AM
Like everything most things, it’s a judgment call, based on asking for feedback from other contributors, listening to Pants users, code review, etc For example,
test --force
was added based on a user request. The other day, I thought “let’s add a `force_reruns`” field to
python_tests()
, and Stu pointed out it was a bit premature, that we don’t know how users would need something like that. So we didn’t land the change.
a

aloof-angle-91616

09/19/2020, 4:03 AM
i don't think the behavior of having to press ^C for every test that fails is good and i think greg said above that he would probably think it should be the default behavior. could you provide an opinion on that?
h

hundreds-father-404

09/19/2020, 4:11 AM
I’m not sure what the right behavior should be. I could see a couple scenarios, including: - “This one test is taking forever. I want to skip it and go on to the next one. <ctrl-c> Oh…I didn’t mean to quit everything.” - “Ah, I didn’t mean to run 100 tests!!! Exit!” I’m really not sure. And possibly an option is indeed appropriate; I didn’t mean to imply that “no way should we do an option”. More, “that might make sense. Idrk. We should generally think really critically before adding more options, but it could be the right decision here.” I apologize that it might have seemed like I was trying to shut down your suggestion.
👍 1
a

aloof-angle-91616

09/19/2020, 4:11 AM
i can make an issue
💯 1
your response above makes it clear you were not trying to shut down my suggestion
❤️ 1
i did not know how to phrase my first response other than slippery slope and i recognize that that's often used in a much harsher way
h

hundreds-father-404

09/19/2020, 4:13 AM
The place I do think I have a preference for is
repl
, that I really like when you’re not using Pantsd that we behave just like you would without Pants. That is, ctrl-c results in “KeyboardInterrupt” being printed to the REPL, but not exiting That would imply “We should pass the signal through to the InteractiveProcess and let it handle it”. But, then, that might be really unergonomic for
test --debug
🤔 and I don’t think we want lots of special casing in the Rust code
a

aloof-angle-91616

09/19/2020, 4:15 AM
does InteractiveProcess consume Process yet? i can perhaps see an argument for adding a field there, which is maybe special casing but it should at least be within an
if
branch predicated on the field
h

hundreds-father-404

09/19/2020, 4:15 AM
and I don’t think we want lots of special casing in the Rust code
Well, possibly, it should be an option on
InteractiveProcess
like
capture_signals
? I’m a little skeptical for similar reasons of “option fatigue”, but this time applied to rule authors. The more parameters we add, the higher our surface area for bugs. But none of these options seem perfect. So, could be worth considering
☝️ 1
✍️ 1
coke
a

aloof-angle-91616

09/19/2020, 4:15 AM
+1
and also if i understand you correctly,
--debug
is especially useful to attach a debugger (and to have stdin and a tty attached, basically): could that be its own goal? feels not too hard because v2 rules are so easy to mix and match
h

hundreds-father-404

09/19/2020, 4:18 AM
Thank you for offering to write an issue. I think that’d help It might be helpful to write out the requirements that we do know, followed by the requirements we do not know Some reqs we do know: *
pantsd
and
--no-pantsd
should be consistent *
repl
and
run
should likely not capture signals; the underlying process should handle them. Note that there’s only one InteractiveProcess in the whole run for those
👖 2
a

aloof-angle-91616

09/19/2020, 4:19 AM
this is fantastic
❤️ 1
h

hundreds-father-404

09/19/2020, 4:20 AM
and also if i understand you correctly, --debug is especially useful to attach a debugger (and to have stdin and a tty attached, basically):
Alex considered that when adding the option a long time ago. Same with Tansy when adding coverage, to have a
coverage
goal. I think a couple people weighed in and all agreed that the option was nice to a) reduce the number of goals that users need to learn, and b) keep things more organized for plugin authors. For example, when you implement
test
, you must implement
test --debug
right now; it would be confusing if you could do one but not the other
some unknown reqs: * What should
test --debug
do? If we let the underlying process handle signals, you would have to press
ctrl-c
for every single test, given that there may be multiple tests in a run. Might an option be appropriate, or options fatigue and we should choose?
If we do have an option, I think that the argument on
InteractiveProcess
would be helpful. We would do something like
capture_signals=test_subsystem.exit_early
a

aloof-angle-91616

09/19/2020, 4:29 AM
For example, when you implement test, you must implement test --debug right now;
something's bugging me, i feel like there's a way to handle this nicely, thinking about it
We would do something like capture_signals=test_subsystem.exit_early
capture_signals
is very interesting. i think that the shell terminal captures it and sends the signal, but i know for example if you run
emacs -nw
and press
C-c
(that's ctrl-c), it obviously captures it normally. that's a curses app, so i don't know at what level that translation occurs, or if we can avoid it being converted into a signal and then handle keyboard codes for the tty or whatever it's called
👀 1
h

hundreds-father-404

09/19/2020, 4:30 AM
something’s bugging me, i feel like there’s a way to handle this nicely, thinking about it
See https://github.com/pantsbuild/example-plugin/blob/main/pants-plugins/examples/bash/shunit2_test_runner.py for how a plugin author implements both.
🙏 1
a

aloof-angle-91616

09/19/2020, 4:31 AM
probably not super important because it works as is i think but not sure
h

hundreds-father-404

09/19/2020, 4:31 AM
I wanted to finish https://www.pantsbuild.org/docs/plugins-test-goal today too, but went into a Pex and MyPy vortex
👀 1
a

aloof-angle-91616

09/19/2020, 4:32 AM
will use the bathroom then i'll review the PR you just posted
h

hundreds-breakfast-49010

09/20/2020, 9:45 AM
thanks for your suggestions on this topic
my original goal in digging into this was to be able to close https://github.com/pantsbuild/pants/issues/10057 . it sounds like what we actually have at hand is a design decision about how specifically
InteractiveProcess
ought to handle signals
- “Ah, I didn’t mean to run 100 tests!!! Exit!” was basically my own reaction when I was first looking into the
test --debug
case, possibly becuase I was operating under the mindset that this was a bug in signal handling, rather than expect, if (possibly) undesirable, behavior