Merge pull request #7234 from p12tic/fix-buildstep-stop-exception

process: Fix exception when buildstep is interrupted while already finishing

master/buildbot/process/buildstep.py

@@ -53,6 +53,7 @@
 from buildbot.process.results import statusToString
 from buildbot.util import bytes2unicode
 from buildbot.util import debounce
+from buildbot.util import deferwaiter
 from buildbot.util import flatten
 from buildbot.util.test_result_submitter import TestResultSubmitter
 from buildbot.warnings import warn_deprecated
@@ -285,6 +286,7 @@ def __init__(self, **kwargs):
         self.stepid = None
         self.results = None
         self._start_unhandled_deferreds = None
+        self._interrupt_deferwaiter = deferwaiter.DeferWaiter()
         self._test_result_submitters = {}
 
     def __new__(klass, *args, **kwargs):
@@ -567,6 +569,9 @@ def setBuildData(self, name, value, source):
 
     @defer.inlineCallbacks
     def _cleanup_logs(self):
+        # Wait until any in-progress interrupt() to finish (that function may add new logs)
+        yield self._interrupt_deferwaiter.wait()
+
         all_success = True
         not_finished_logs = [v for (k, v) in self.logs.items() if not v.finished]
         finish_logs = yield defer.DeferredList([v.finish() for v in not_finished_logs],
@@ -641,8 +646,13 @@ def _maybe_interrupt_cmd(self, reason):
         except Exception as e:
             log.err(e, 'while cancelling command')
 
-    @defer.inlineCallbacks
     def interrupt(self, reason):
+        # Note that this method may be run outside usual step lifecycle (e.g. after run() has
+        # already completed), so extra care needs to be taken to prevent race conditions.
+        return self._interrupt_deferwaiter.add(self._interrupt_impl(reason))
+
+    @defer.inlineCallbacks
+    def _interrupt_impl(self, reason):
         if self.stopped:
             # If we are in the process of interruption and connection is lost then we must tell
             # the command not to wait for the interruption to complete.
@@ -656,12 +666,8 @@ def interrupt(self, reason):
                 lock.stopWaitingUntilAvailable(self, access, d)
             self._acquiringLocks = []
 
-        if self._waitingForLocks:
-            yield self.addCompleteLog(
-                'cancelled while waiting for locks', str(reason))
-        else:
-            yield self.addCompleteLog('cancelled', str(reason))
-
+        log_name = "cancelled while waiting for locks" if self._waitingForLocks else "cancelled"
+        yield self.addCompleteLog(log_name, str(reason))
         yield self._maybe_interrupt_cmd(reason)
 
     def releaseLocks(self):

master/buildbot/test/unit/process/test_buildstep.py

@@ -410,12 +410,13 @@ def test_lost_remote_during_interrupt(self):
 
         @defer.inlineCallbacks
         def on_command(cmd):
-            cmd.conn.set_expect_interrupt()
-            cmd.conn.set_block_on_interrupt()
+            conn = cmd.conn
+            conn.set_expect_interrupt()
+            conn.set_block_on_interrupt()
             d1 = step.interrupt('interrupt reason')
             d2 = step.interrupt(failure.Failure(error.ConnectionLost()))
 
-            cmd.conn.unblock_waiters()
+            conn.unblock_waiters()
             yield d1
             yield d2
 

newsfragments/build-cancel-exceptions.bugfix

@@ -0,0 +1 @@
+Fixed a race condition resulting in ``EXCEPTION`` build results when build steps that are about to end are cancelled.