even with the above PR if you run ` pants repl` then hit ctr

Question

even with the above PR if you run ` pants repl` then hit ctrl c some kind of python process will remain around and screwing up the terminal

hundreds-breakfast-49010 · Answer

specifically it s ` home gregs code pants build support virtualenvs Linux pants dev deps py38 venv bin python3 home gregs code pants pants d tmp0rx6pokk requirements pex` which is I think the actual process that runs when pants creates a python repl I m not sure why this is getting killed when the pytest process is getting killed with the above commit

hundreds-breakfast-49010 · Answer

anyway like I said earlier I am getting rather frustrated with dealing with the signal handling code between the no pantsd and pantsd cases and I would like to do what I can to get rid of that code I don t think <https github com pantsbuild pants issues 10061> is necessarily related to this

hundreds-breakfast-49010 · Answer

that is to say if I understand correctly porting the nailgun client to rust is orthogonal to the problem of replacing signal communication with the nailgun protocol heartbeat system that <https github com pantsbuild pants pull 10320> mentions

witty-crayon-22786 · Answer

correct

witty-crayon-22786 · Answer

sorry i think i said i was going to sketch out the heartbeat stuff a bit better on the ticket

witty-crayon-22786 · Answer

will follow up to do that

hundreds-breakfast-49010 · Answer

awesome

hundreds-breakfast-49010 · Answer

yeah I think implementing that heartbeat system would do a lot to make reasoning about how signaling happens easier

hundreds-father-404 · Answer

That sounds like a great investment Thanks for all your improvements on this Greg And to < aloof angle 91616> for the original implementation taking us so far This is tricky stuff

witty-crayon-22786 · Answer

maybe the way i would suggest thinking about it is a signal arriving at the server currently sets a signal to say that any ongoing work should exit in the future a heartbeat arriving at the server would set a per session signal that everything in that session should exit

witty-crayon-22786 · Answer

signals are definitely fidgety and limited though

aloof-angle-91616 · Answer

i definitely think that sending a SIGKILL to the PGRP of the pants process so negative of its own PID right before it exits would solve some of these issues with leftover processes without a lot of engineering effort

witty-crayon-22786 · Answer

yea that s worth looking at < hundreds breakfast 49010>

hundreds-breakfast-49010 · Answer

< aloof angle 91616> at some point today I tried sending signals to the PGRP and it had unexpected bad behavior and I forget exactly what that behavior was

witty-crayon-22786 · Answer

iff the right behavior is always to kill the whole tree which i think it might not be in the case of `run` and `repl`

hundreds-breakfast-49010 · Answer

so I abandoned that plan

aloof-angle-91616 · Answer

oh cool i ll try it out i haven t yet

hundreds-breakfast-49010 · Answer

I think what might ve happened is that sending a signal to the PGRP kills everything including the pantsd process itself and that meant that there was cleanup that was supposed to happen but didn t

aloof-angle-91616 · Answer

oh yes it would have to be at the very very end

hundreds-breakfast-49010 · Answer

but I didn t investigate it much I just moved on and used the `children` thing `psutil` gives you

aloof-angle-91616 · Answer

the pgrp seems less prone to race conditions is all

witty-crayon-22786 · Answer

it sounds like the challenge is that you don t want to kill pantsd in the repl case that s going to be challenging even especially with the heartbeat strategy

aloof-angle-91616 · Answer

yes that would be much more difficult

hundreds-breakfast-49010 · Answer

I think we re killing pantsd more than we need to ideally you should be able to kill the client and have pantsd continue to run without a problem right

witty-crayon-22786 · Answer

gt that s going to be challenging even especially with the heartbeat strategy to clarify it s easy to not kill pantsd with the heartbeat strategy but it s challenging for the server to tear down processes cleanly

witty-crayon-22786 · Answer

< hundreds breakfast 49010> no

witty-crayon-22786 · Answer

that is one of the key reasons for switching to heartbeats

aloof-angle-91616 · Answer

i will review the heartbeat strategy

witty-crayon-22786 · Answer

< hundreds breakfast 49010> to be clear with heartbeats `pantsd` should stay up but the run associated with the client should definitely die

witty-crayon-22786 · Answer

let me dump the explanation right now

aloof-angle-91616 · Answer

it was perhaps easier to conceptualize when the daemon forked to form the pantsd runner

witty-crayon-22786 · Answer

yea

witty-crayon-22786 · Answer

i mean it s still easy to conceptualize i think if you re ok with killing everything which was the strategy until recently

aloof-angle-91616 · Answer

yes which would be the only justification for the PGRP upon death

witty-crayon-22786 · Answer

ok posted the sketch of the heartbeat stuff on <https github com pantsbuild pants issues 10057> but typing that up actually made me think of something else so maybe hold off on even reading it

witty-crayon-22786 · Answer

< hundreds breakfast 49010> posting that reminded me of how we handled cancelation of nodes which was to add a drop guard it feels like having a drop guard for the InteractiveProcess that kills the child might help or perhaps only be necessary in the future to implement the heartbeat strategy

hundreds-breakfast-49010 · Answer

I ll keep that strategy in mind

witty-crayon-22786 · Answer

i guess that IP is not currently async though would have to make it async to allow for interrupting via drop

aloof-angle-91616 · Answer

that would be pretty neat

aloof-angle-91616 · Answer

left a comment on possible extensions to the heartbeat strategy by reading the raw tty and or opening up a pty if the simpler version still has issues with `InteractiveProcess` and `^C` i hope we don t have to use either of them <https github com pantsbuild pants issues 10057 issuecomment 698845410>