launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/process-with-timeout-bug-629378 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #629378 ProcessWithTimeout.timeoutConnection has a race condition
  https://bugs.launchpad.net/bugs/629378


= Summary =
Catch a race condition in ProcessWithTimeout

== Proposed fix ==
Catch `ProcessExitedAlready` in ProcessWithTimeout.timeoutConnection().

This can happen if the process has exited normally at about the same time as 
the timer for the timeout fires to kill the process.

== Implementation details ==
The code change is simple.  The tests use a stub process transport which 
simulates a process.  Because of this, the "real" processEnded is never called 
so we can't chain a callback to test that it has run to completion.  Right now 
we just test that the deferred was not called at all, which is lame but the 
test blows up on the clock advance anyway without the fix.

== Tests ==
bin/test -cvvt TestProcessWithTimeout
-- 
https://code.launchpad.net/~julian-edwards/launchpad/process-with-timeout-bug-629378/+merge/36279
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/process-with-timeout-bug-629378 into lp:launchpad/devel.

=== modified file 'lib/lp/services/twistedsupport/processmonitor.py'
--- lib/lp/services/twistedsupport/processmonitor.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/twistedsupport/processmonitor.py	2010-09-22 10:51:08 +0000
@@ -269,8 +269,10 @@
     """Run a process and capture its output while applying a timeout."""
 
     # XXX Julian 2010-04-21
-    # This class doesn't have any unit tests yet, it's used by
+    # This class doesn't have enough unit tests yet, it's used by
     # lib/lp/buildmaster/manager.py which tests its features indirectly.
+    # See lib/lp/services/twistedsupport/tests/test_processmonitor.py -
+    # TestProcessWithTimeout for the beginnings of tests.
 
     def __init__(self, deferred, timeout, clock=None):
         self._deferred = deferred
@@ -312,7 +314,11 @@
 
     def timeoutConnection(self):
         """When a timeout occurs, kill the process with a SIGKILL."""
-        self._process_transport.signalProcess("KILL")
+        try:
+            self._process_transport.signalProcess("KILL")
+        except error.ProcessExitedAlready:
+            # The process has already died. Fine.
+            pass
         # processEnded will get called.
 
     def processEnded(self, reason):

=== modified file 'lib/lp/services/twistedsupport/tests/test_processmonitor.py'
--- lib/lp/services/twistedsupport/tests/test_processmonitor.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/twistedsupport/tests/test_processmonitor.py	2010-09-22 10:51:08 +0000
@@ -24,6 +24,7 @@
     ProcessMonitorProtocol,
     ProcessMonitorProtocolWithTimeout,
     ProcessProtocolWithTwoStageKill,
+    ProcessWithTimeout,
     run_process_with_timeout,
     )
 
@@ -98,6 +99,40 @@
         self.protocol.connectionMade()
 
 
+class TestProcessWithTimeout(ProcessTestsMixin, TrialTestCase):
+    """Tests for `ProcessWithTimeout`."""
+
+    layer = TwistedLayer
+    TIMEOUT = 100 # arbitrary, big enough not to really fire.
+
+    def makeProtocol(self):
+        """See `ProcessMonitorProtocolTestsMixin.makeProtocol`."""
+        self._deferred = defer.Deferred()
+        return ProcessWithTimeout(
+            self._deferred, self.TIMEOUT, clock=self.clock)
+
+    def test_end_versus_timeout_race_condition(self):
+        # If the timeout fires around the same time as the process ends,
+        # then there is a race condition.  The timeoutConnection()
+        # method can try and kill the process which has already exited
+        # which normally throws a
+        # twisted.internet.error.ProcessExitedAlready - the code should
+        # catch this and ignore it.
+
+        # Simulate process exit without "killing" it:
+        self.protocol._process_transport = self.protocol.transport
+        self.protocol.transport.exited = True
+
+        # Without catching the ProcessExitedAlready this will blow up.
+        self.clock.advance(self.TIMEOUT+1)
+
+        # At this point, processEnded is yet to be called so the
+        # Deferred has not fired.  Ideally it would be nice to test for
+        # something more concrete here but the stub transport doesn't
+        # work exactly like the real one.
+        self.assertFalse(self._deferred.called)
+
+
 class TestProcessProtocolWithTwoStageKill(
     ProcessTestsMixin, TrialTestCase):