Better handle startup failures - do not leave dangling processes (#201)

* Move process killing to a separate method

This allows calling just the killing part of quit in a subsequent
commit. This only moves some code, without changing behaviour.

* Terminate process on startup errors

Before, if a process was successfully started, but there would be a
problem in subsequent setup (in particular the dbus connection could
fail), the process could keep running in the background.

This was particularly problematic when this caused the OMXPlayer
constructor to raise an exception, since then no player instance would
be created so the process pid would be lost.

This commit ensures that whenever the OMXPlayer constructor or load
method throws, no process will be dangling in the background.

This fixes #196

* Add more tests for player.quit

This adds test coverage for two exception handling cases.

Author: matthijskooijman

omxplayer/player.py

@@ -190,10 +190,15 @@ def monitor(self, process, on_exit):
                                    stdin=devnull,
                                    stdout=devnull,
                                    preexec_fn=os.setsid)
-        self._process_monitor = threading.Thread(target=monitor,
-                                                 args=(self, process, on_exit))
-        self._process_monitor.start()
-        return process
+        try:
+            self._process_monitor = threading.Thread(target=monitor,
+                                                     args=(self, process, on_exit))
+            self._process_monitor.start()
+            return process
+        except:
+            # Make sure to not leave any dangling process on failure
+            self._terminate_process(process)
+            raise
 
     def _setup_omxplayer_process(self, source):
         logger.debug('Setting up OMXPlayer process')
@@ -205,6 +210,14 @@ def _setup_omxplayer_process(self, source):
         atexit.register(self.quit)
         return process
 
+    def _terminate_process(self, process):
+        try:
+            process_group_id = os.getpgid(process.pid)
+            os.killpg(process_group_id, signal.SIGTERM)
+            logger.debug('SIGTERM Sent to pid: %s' % process_group_id)
+        except OSError:
+            logger.error('Could not find the process to kill')
+
     def _setup_dbus_connection(self, Connection, bus_address_finder):
         logger.debug('Trying to connect to OMXPlayer via DBus')
         tries = 0
@@ -234,10 +247,17 @@ def load(self, source, pause=False):
             source (string): Path to the file to play or URL
         """
         self._source = source
-        self._load_source(source)
-        if pause:
-            time.sleep(0.5)  # Wait for the DBus interface to be initialised
-            self.pause()
+        try:
+            self._load_source(source)
+            if pause:
+                time.sleep(0.5)  # Wait for the DBus interface to be initialised
+                self.pause()
+        except:
+            # Make sure we do not leave any dangling process
+            if self._process:
+                self._terminate_process(self._process)
+                self._process = None
+            raise
 
     """ ROOT INTERFACE PROPERTIES """
 
@@ -846,15 +866,10 @@ def quit(self):
         if self._process is None:
             logger.debug('Quit was called after self._process had already been released')
             return
-        try:
-            logger.debug('Quitting OMXPlayer')
-            process_group_id = os.getpgid(self._process.pid)
-            os.killpg(process_group_id, signal.SIGTERM)
-            logger.debug('SIGTERM Sent to pid: %s' % process_group_id)
-            self._process_monitor.join()
-        except OSError:
-            logger.error('Could not find the process to kill')
 
+        logger.debug('Quitting OMXPlayer')
+        self._terminate_process(self._process)
+        self._process_monitor.join()
         self._process = None
 
     @_check_player_is_active

tests/unit/test_omxplayer.py

@@ -46,6 +46,24 @@ def test_tries_to_open_dbus_again_if_it_cant_connect(self, *args):
             self.patch_and_run_omxplayer(Connection=dbus_connection)
             self.assertEqual(50, self.player.tries)
 
+    def test_dbus_failure_kills(self, popen, sleep, isfile, killpg, *args):
+        omxplayer_process = Mock()
+        popen.return_value = omxplayer_process
+        dbus_connection = Mock(side_effect=DBusConnectionError)
+        with patch('os.getpgid', Mock(return_value=omxplayer_process.pid)):
+            with self.assertRaises(SystemError):
+                self.patch_and_run_omxplayer(Connection=dbus_connection)
+            killpg.assert_called_once_with(omxplayer_process.pid, signal.SIGTERM)
+
+    def test_thread_failure_kills(self, popen, sleep, isfile, killpg, *args):
+        omxplayer_process = Mock()
+        popen.return_value = omxplayer_process
+        with patch ('threading.Thread', Mock(side_effect=RuntimeError)):
+            with patch('os.getpgid', Mock(return_value=omxplayer_process.pid)):
+                with self.assertRaises(RuntimeError):
+                    self.patch_and_run_omxplayer()
+                killpg.assert_called_once_with(omxplayer_process.pid, signal.SIGTERM)
+
     @parameterized.expand([
         ['can_quit', 'CanQuit', [], []],
         ['can_set_fullscreen', 'CanSetFullscreen', [], []],
@@ -128,6 +146,27 @@ def test_quitting_waits_for_omxplayer_to_die(self, popen, *args):
             self.player.quit()
             omxplayer_process.wait.assert_has_calls([call()])
 
+    def test_quitting_when_already_dead(self, popen, sleep, isfile, killpg, *args):
+        omxplayer_process = Mock()
+        popen.return_value = omxplayer_process
+        self.patch_and_run_omxplayer()
+        # Pretend the process is already dead
+        killpg.configure_mock(side_effect=OSError)
+        with patch('os.getpgid', Mock(return_value=omxplayer_process.pid)):
+            # This tests that quit handles the OSError
+            self.player.quit()
+            killpg.assert_called_once_with(omxplayer_process.pid, signal.SIGTERM)
+
+    def test_quitting_twice(self, popen, sleep, isfile, killpg, *args):
+        omxplayer_process = Mock()
+        popen.return_value = omxplayer_process
+        self.patch_and_run_omxplayer()
+        with patch('os.getpgid', Mock(return_value=omxplayer_process.pid)):
+            # This should not raise, and call killpg once
+            self.player.quit()
+            self.player.quit()
+            killpg.assert_called_once_with(omxplayer_process.pid, signal.SIGTERM)
+
     def test_check_process_still_exists_before_dbus_call(self, *args):
         self.patch_and_run_omxplayer()
         self.player._process = process = Mock(return_value=None)