Merge lp:~gocept/landscape-client/raise-error-on-signal into lp:~landscape/landscape-client/trunk

[Steffen Allner]

"Raise" a new SignalError from AllOutputProcessProtocol.processEnded().

The exception is propagated through reactor.errback() rather than actually raised. SignalError happens when a twisted-spawned process exits due to a signal. This is a necessary change because of how twisted errbacks are handled in Python 3. However, it is also a valid change for Python 2 since errbacks align with exceptions, not tuples.

As was mentioned in the comment here by Adam [0], it is necessary, that we issue a proper Exception instead of a tuple if we want to be catched by an errback.

As this code path was not tested before, I added a unittest to cover this.

[0] https://code.launchpad.net/~gocept/landscape-client/py3-manager-shutdownmanager-aptsources/+merge/321253/comments/841470

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make ci-check
Result: Success
Revno: 997
Branch: lp:~gocept/landscape-client/raise-error-on-signal
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3827/

review: Approve (test results)

[lp:~gocept/landscape-client/raise-error-on-signal]

998. By Steffen Allner on 2017-04-03

Backmerge from trunk.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make ci-check
Result: Success
Revno: 998
Branch: lp:~gocept/landscape-client/raise-error-on-signal
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3828/

review: Approve (test results)

[Eric Snow (ericsnowcurrently)]

This patch looks good, but should include changes for the test that motivated this change:

  landscape.manager.tests.test_aptsources.AptSourcesTests.test_signaled_import_reported

review: Needs Fixing

[Steffen Allner (sallner)]

> This patch looks good, but should include changes for the test that motivated
> this change:
>
> landscape.manager.tests.test_aptsources.AptSourcesTests.test_signaled_import
> _reported

Hm, I tried different things to do something like an integration test, but failed to achieve this. I have probably not fully understood how the test case operates. The closest I got was [0]. If I just return the spawn_process() call in _run_process() as in the actual implement the test fails, probably because it does not support async in this way.

Maybe I also misunderstood you and you just requested a renaming of the RuntimeError to a SignalError.

Nevertheless, I would be happy to understand the way the tests interact here.

[0] http://paste.ubuntu.com/24313000/

[Eric Snow (ericsnowcurrently)]

Right, I only meant changing RuntimeError to SignalError. :)

[Daniel Havlik (nilo)]

+1

review: Approve

[lp:~gocept/landscape-client/raise-error-on-signal]

999. By Steffen Allner on 2017-04-05

Change the error to SignalError, which is actually raised in case of a process terminated by a signal in l.l.twisted_util.

[Steffen Allner (sallner)]

> Right, I only meant changing RuntimeError to SignalError. :)

That simplifies things ;) Changed and pushed.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-check
Result: Success
Revno: 999
Branch: lp:~gocept/landscape-client/raise-error-on-signal
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3835/

review: Approve (test results)

[Eric Snow (ericsnowcurrently)]

LGTM :)

review: Approve

=== modified file 'landscape/lib/tests/test_twisted_util.py'
--- landscape/lib/tests/test_twisted_util.py	2017-03-14 09:53:55 +0000
+++ landscape/lib/tests/test_twisted_util.py	2017-04-05 07:25:59 +0000
@@ -136,3 +136,27 @@
         result = spawn_process(self.command, stdin="hello")
         result.addCallback(callback)
         return result
+
+    def test_spawn_process_with_signal(self):
+        """
+        In case the process gets terminated by a signal, it raises a
+        C{SignalError}.
+        """
+        # This script will kill itself.
+        create_text_file(self.command, """\
+#!/bin/sh
+kill $$
+""")
+
+        def callback(args):
+            raise RuntimeError("Errback was not called.")
+
+        def errback(failure):
+            out, err, code = failure.value.args
+            self.assertEqual(out, b"")
+            self.assertEqual(err, b"")
+            self.assertEqual(code, 15)  # 15 for SIGTERM
+
+        result = spawn_process(self.command)
+        result.addCallbacks(callback, errback)
+        return result

=== modified file 'landscape/lib/twisted_util.py'
--- landscape/lib/twisted_util.py	2017-04-04 08:50:05 +0000
+++ landscape/lib/twisted_util.py	2017-04-05 07:25:59 +0000
@@ -4,11 +4,16 @@
 from twisted.internet.protocol import ProcessProtocol
 from twisted.internet.process import Process, ProcessReader
 from twisted.internet import reactor
+from twisted.python.failure import Failure
 from twisted.python.compat import itervalues, networkString
 
 from landscape.lib.encoding import encode_values
 
 
+class SignalError(Exception):
+    """An error if the process was terminated by a signal."""
+
+
 def gather_results(deferreds, consume_errors=False):
     d = DeferredList(deferreds, fireOnOneErrback=1,
                      consumeErrors=consume_errors)
@@ -59,7 +64,8 @@
         e = reason.value
         code = e.exitCode
         if e.signal:
-            self.deferred.errback((out, err, e.signal))
+            failure = Failure(SignalError(out, err, e.signal))
+            self.deferred.errback(failure)
         else:
             self.deferred.callback((out, err, code))
 

=== modified file 'landscape/manager/tests/test_aptsources.py'
--- landscape/manager/tests/test_aptsources.py	2017-03-29 11:38:02 +0000
+++ landscape/manager/tests/test_aptsources.py	2017-04-05 07:25:59 +0000
@@ -8,7 +8,7 @@
 from landscape.manager.aptsources import AptSources
 from landscape.manager.plugin import SUCCEEDED, FAILED
 
-from landscape.lib.twisted_util import gather_results
+from landscape.lib.twisted_util import gather_results, SignalError
 from landscape.tests.helpers import LandscapeTest, ManagerHelper
 from landscape.package.reporter import find_reporter_command
 
@@ -356,7 +356,7 @@
         deferred = Deferred()
 
         def _run_process(command, args, env={}, path=None, uid=None, gid=None):
-            deferred.errback(RuntimeError("nok", "some error", 1))
+            deferred.errback(SignalError("nok", "some error", 1))
             return deferred
 
         self.sourceslist._run_process = _run_process