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

Cool :)

[1]

+ def _get_child_connect_timeout(self):
+ """signal.alarm only supports 1s granularity.
+
+ We have to make sure we don't ever send 0, which would not generate an
+ alarm.
+ """
+ timeout = int(self._child_connect_timeout)
+ if timeout <= 0:
+ timeout = 1
+ return timeout

I can't see where _child_connect_timeout might be set to something
other than an int, so you could remove this and prevent problems
another way:

        signal.alarm(max(1, self._child_connect_timeout))

But I don't really have a strong argument why you ought to change it
other than it's more code to read, more stuff to brain-load when
coming in to maintain this code.

[2]

+ # Even though it timed out, we still cleanup the temp dir
+ self.assertFalse(os.path.exists(tempdir))

I think this only happens because SIGALRM was neutered. If no handler
was installed I doubt the temporary directory would be cleared
out. Perhaps there needs to be a handler for SIGALRM anyway, that
raises a custom exception perhaps, or signal.default_int_handler, or
whatever :)

[3]

+ # We can't actually run this test, because by default 'bzr serve
+ # --inet' does not flush after each message.

Out of interest, why doesn't it flush? Can this test be fixed and
enabled later? Consider removing it if it's just going to bit-rot.

[4]

+ # The master process should clean up after the now deceased child.
+ self.failIfExists(path)

If, for some reason, the master believes the child has failed, but
there's just been a misunderstanding and the child is really still
running, could this mean that both the master and the child try to
clean up the fifos directory? _cleanup_fifos() in the child code
doesn't swallow errors, so this might fail.

Consider using shutil instead:

  shutil.rmtree(path, ignore_errors=True)