[Buildbot-devel] Slave not recognizing process end

[exarkun at twistedmatrix.com]

On 12:17 am, martin at v.loewis.de wrote:
>>>The code that's calling signalProcess('KILL') should handle that
>>>exception.
>>>
>>>The exception is present all the way back to Twisted-2.5.0, so it
>>>should be safe to catch.
>>
>>Why handle the exception rather than changing the code to not try to
>>kill processes after they've already exited?  There is no race
>>condition.  It's possibly to do this correctly.
>
>I'm not too familiar with the code, but I find that the race condition
>is fairly obvious. Something (either the timeout, or the master) may
>chose to kill the process, and after the start of processing the
>request, before the signal is sent, the process terminates.
>
>How exactly would it be done correctly?

I may have been unclear when I said "no race".  Since there is more than 
one thing happening, there are at least two different orders they may 
happen in, so in that sense there is a race.  What I was really trying 
to convey is that it's always possible to *know* which order the events 
have happened in, without resorting to handling the exception.

I'm also not extremely familiar with the code, but let's say that 
there's a timeout (which I think there is) which tries to kill the 
process.  To avoid the exception, the callback for handling process 
termination (with or without error) should cancel the timeout.  Thus, no 
attempt is made by the timeout-related code to kill the process.  If 
there are other bits of code which would try to kill the process, 
perhaps they can have their event sources aborted as well.

The main reason this is better than just handling the exception is that 
it fits right in with the process tracking that BuildBot really should 
already be doing.  If it is going off and killing a process that exited 
already, then the obvious question to ask is why it didn't notice the 
process already exited, and what consequences this has for any related 
status view and other internal state tracking.

Jean-Paul