Better handle startup failures - do not leave dangling processes #201
Conversation
This allows calling just the killing part of quit in a subsequent commit. This only moves some code, without changing behaviour.
Codecov Report
@@ Coverage Diff @@
## master #201 +/- ##
==========================================
+ Coverage 72.05% 73.82% +1.77%
==========================================
Files 6 6
Lines 458 470 +12
Branches 25 26 +1
==========================================
+ Hits 330 347 +17
+ Misses 117 113 -4
+ Partials 11 10 -1 Continue to review full report at Codecov.
|
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 willprice#196
This adds test coverage for two exception handling cases.
matthijskooijman
force-pushed
the
cleanup-startup-failure
branch
from
2dc891e to
932e1db
Compare
|
w00ps, misclicked the close button. Anyway, based on the code coverage report, I added three more testcases (one amended into the second comment, two unrelated ones added in a separate commit). Code coverage should now improve rather then be reduced by this PR :-) |
|
Thanks for this @matthijskooijman, it looks really good. I don't have much time this week to look at this. I'd like to test it out myself before merging. I'll try and do that next week. Thanks again for your contribution! |
I know very well how that works, so don't worry (but thanks for replying anyway!) |
| args=(self, process, on_exit)) | ||
| self._process_monitor.start() | ||
| return process | ||
| except: |
There was a problem hiding this comment.
Is there any chance we can make this except block a bit more specific rather than just a catch all? Can we catch ThreadError and/or RuntimeError?
There was a problem hiding this comment.
I realize that the common wisdom is to catch errors specifically, but that only applies when your handling code responds to a particular error. In this case, the handling code is cleanup that should run whenever the code in question raises an exception, regardless of which one exactly (since then the contract is: When an exception is raised, no process is left running).
| 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') |
There was a problem hiding this comment.
Under what circumstances might we get an OSError? If process_group_id no longer exists by the time os.killpg occurs?
There was a problem hiding this comment.
I think I've seen exactly that, yes. Also, this is just code that was already there, I just moved it. :-)
| if pause: | ||
| time.sleep(0.5) # Wait for the DBus interface to be initialised | ||
| self.pause() | ||
| except: |
There was a problem hiding this comment.
Same comments as above RE exception specificity
| if self._process: | ||
| self._terminate_process(self._process) | ||
| self._process = None | ||
| raise |
There was a problem hiding this comment.
Don't we need to catch the exception, get the traceback info via sys.exc_info() and reraise that (see https://stackoverflow.com/questions/18188563/how-to-re-raise-an-exception-in-nested-try-except-blocks)
There was a problem hiding this comment.
I think Python handles this already by saying something like "While handling this exception, another exception occured", or "WHich was the direct cause of this exception", something like that. We could try do reproduce the original exception exactly, but that might just make debugging more complicated if it looks like the exception handler never ran?
There was a problem hiding this comment.
Yep, you're right, I didn't realise this was also supported in py 2.7 (https://docs.python.org/2.7/tutorial/errors.html section 8.3) 👍
|
Thanks again @matthijskooijman. This is great and thanks for putting the effort into writing the tests 👍. I've pulled down your branch and run the tests and things seem good (although there are a few spurious errors in the integration tests, I don't think they're anything to do with your changes, I'll have a look into them separately). Sorry it's taken me so long to look at this, but I hope to merge this in this week :) |
Description
This PR fixes #196, by making sure that if any startup failures occur after the omxplayer process is started, that the omxplayer process is killed before raising an exception. This prevents a situation where an omxplayer might be running in the background, without any reference to its pid to kill or control it.
Todos
Steps to Test or Reproduce
Without this commit, if you let
DBusConnection.__init__raise DBusException("Foo")and then start a player, the constructor will throw an exception but leave omxplayer running in the background.