Code review comment for lp:~jameinel/launchpad/lp-serve-child-hangup

On 4 March 2011 18:00, John Arbash Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 3/4/2011 4:10 AM, Martin Pool wrote:
>> That looks pretty good to me.  It's a bit simpler than having a separate thread, and I would say runs less risk of unexpected environmental problems.
>>
>> One more question:
>>
>> +        for path, flags in to_open:
>> +            try:
>> +                fids.append(os.open(path, flags))
>> +            except OSError, e:
>> +                if e.errno == errno.EINTR:
>> +                    error = ('After %.3fs we failed to open %s, exiting'
>> +                             % (time.time() - tstart, path,))
>> +                    trace.warning(error)
>> +                    for fid in fids:
>> +                        try:
>> +                            os.close(fid)
>> +                        except OSError:
>> +                            pass
>> +                    self._cleanup_fifos(base_path)
>> +                    raise errors.BzrError(error)
>> +                raise
>>
>> It seems to me that normally you won't actually hit that except block, because the sigalarm will normally immediately terminate the process.  Am I wrong?  However, you will hit it when it's run from a test where you do set a handler.  I think this behaviour is ok, but I would like a comment in the except block warning that it doesn't necessarily run.
>>
>> (Alternatively, on the main code path, you could set sigalarm to a do-nothing handler, which would probably suffice to get you an eintr.  But maybe not: istr mgz discovering that Python has some strange behaviour about automatically restarting some system calls.  So on the whole I wouldn't do that.)
>>
>> so, from me, just 'tweak' to add a comment.  annelap may care to review it again.
>

> Setting SIGALRM to a do-nothing handler does, indeed, cause me to get
> EINTR. There is a test that already exercises it, because it is what was
> used with the old SIGUSR1 handler.
>
> Would you rather just see it removed, since SIGALRM is killing the child
> completely by default?

My order of preference is

1- remove it and count on sigalrm actually killing it, including from the test
2- leave it and just add a comment it's not normally hit
3- leave it and also register a do-nothing handler to make sure it
actually is hit

I don't like 2 and 3 so much because there's some chance it will break
if something else changes, and there may be other reasons we get an
eintr earlier than we actually wanted to time out. (The latter is not
very likely admittedly.) Also, all else being equal, less code is
better.