Merge lp:~nataliabidart/ubuntu-sso-client/dont-let-it-go into lp:ubuntu-sso-client

[Natalia Bidart]

- Hold on to the Qprocess instance to avoid garbage collection (LP: #930140).

[Roberto Alsina (ralsina)]

+1 except for style nitpick discussed on IRC

review: Approve

[lp:~nataliabidart/ubuntu-sso-client/dont-let-it-go]

857. By Natalia Bidart on 2012-02-10

- Better reading.

858. By Natalia Bidart on 2012-02-10

Also handle removing the process on error.

[dobey (dobey)]

review: Approve

=== modified file 'ubuntu_sso/utils/runner/glib.py'
--- ubuntu_sso/utils/runner/glib.py	2012-02-07 20:57:15 +0000
+++ ubuntu_sso/utils/runner/glib.py	2012-02-10 15:52:57 +0000
@@ -16,6 +16,8 @@
 
 """Utility to spawn another program from a GLib mainloop."""
 
+import os
+
 # pylint: disable=E0611,F0401
 try:
     from shlex import quote
@@ -49,7 +51,14 @@
         # pylint: disable=E1103
         GLib.spawn_close_pid(pid)
         # pylint: enable=E1103
-        reply_handler(status)
+
+        if os.WIFEXITED(status):
+            status = os.WEXITSTATUS(status)
+            reply_handler(status)
+        else:
+            msg = 'Child terminated abnormally, '\
+                  'status from waitpid is %r' % status
+            error_handler(msg=msg, failed_to_start=False)
 
     def handle_error(gerror):
         """Handle error when spawning the process."""

=== modified file 'ubuntu_sso/utils/runner/qt.py'
--- ubuntu_sso/utils/runner/qt.py	2012-01-26 18:58:58 +0000
+++ ubuntu_sso/utils/runner/qt.py	2012-02-10 15:52:57 +0000
@@ -23,6 +23,11 @@
 
 logger = setup_logging("ubuntu_sso.utils.runner.qt")
 
+# pylint: disable=C0103
+# the set below is a workaround to hold to object references to avoid
+# garbage collection before the processes finish executing
+_processes = set()
+
 
 def spawn_program(args, reply_handler, error_handler):
     """Spawn the program specified by 'args' using the Qt mainloop.
@@ -36,6 +41,7 @@
     """
 
     process = QtCore.QProcess()
+    _processes.add(process)
 
     def print_pid():
         """Add a debug log message."""
@@ -43,14 +49,20 @@
         logger.debug('Spawning the program %r with the qt mainloop '
                      '(returned pid is %r).', args, pid)
 
+    def child_watch(status):
+        """Handle child termination."""
+        reply_handler(status)
+        _processes.remove(process)
+
     def handle_error(process_error):
         """Handle error when spawning the process."""
-        failed_to_start = process_error == QtCore.QProcess.FailedToStart
+        failed_to_start = (process_error == process.FailedToStart)
         msg = 'ProcessError is %r' % process_error
         error_handler(msg=msg, failed_to_start=failed_to_start)
+        _processes.remove(process)
 
     process.started.connect(print_pid)
-    process.finished.connect(reply_handler)
+    process.finished.connect(child_watch)
     process.error.connect(handle_error)
 
     args = list(args)

=== modified file 'ubuntu_sso/utils/runner/tests/test_qt.py'
--- ubuntu_sso/utils/runner/tests/test_qt.py	2012-02-08 16:35:50 +0000
+++ ubuntu_sso/utils/runner/tests/test_qt.py	2012-02-10 15:52:57 +0000
@@ -22,6 +22,7 @@
 from twisted.internet import defer
 
 from ubuntu_sso.utils import runner
+from ubuntu_sso.utils.runner import qt
 from ubuntu_sso.utils.runner.tests.test_runner import SpawnProgramTestCase
 
 
@@ -88,6 +89,48 @@
         yield super(QtSpawnProgramTestCase, self).setUp()
         # Since we can't mix plan qt runner and the qt4reactor, we patch
         # QProcess and fake the conditions so the qt runner is chosen
-        self.patch(QtCore, 'QProcess', FakedProcess)
+        self.process = FakedProcess()
+        self.patch(QtCore, 'QProcess', lambda: self.process)
         self.patch(runner, 'is_twisted_reactor_installed', lambda: False)
         self.patch(runner, 'is_qt4_main_loop_installed', lambda: True)
+
+    @defer.inlineCallbacks
+    def test_dont_let_it_go_success(self):
+        """The QProcess instance is stored (and removed) to avoid GC."""
+        # pylint: disable=W0212
+
+        d = defer.Deferred()
+
+        def check(status):
+            """Do the check."""
+            self.assertIn(self.process, qt._processes)
+            d.callback(status)
+
+        runner.qt.spawn_program(self.args,
+            reply_handler=check, error_handler=lambda **kw: d.errback(kw))
+
+        yield d
+
+        self.assertNotIn(self.process, qt._processes)
+
+    @defer.inlineCallbacks
+    def test_dont_let_it_go_error(self):
+        """The QProcess instance is stored (and removed) to avoid GC."""
+        # pylint: disable=W0212
+
+        d = defer.Deferred()
+
+        def check(msg, failed_to_start):
+            """Do the check."""
+            self.assertIn(self.process, qt._processes)
+            d.callback(msg)
+
+        self.process._error = 42  # this will make the process emit 'error'
+
+        runner.qt.spawn_program(self.args,
+            reply_handler=lambda _: d.errback(AssertionError(_)),
+            error_handler=check)
+
+        yield d
+
+        self.assertNotIn(self.process, qt._processes)