Merge lp://staging/~cjwatson/rabbitfixture/signal-esrch into lp://staging/rabbitfixture

Proposed by Colin Watson
Status: Merged
Merged at revision: 55
Proposed branch: lp://staging/~cjwatson/rabbitfixture/signal-esrch
Merge into: lp://staging/rabbitfixture
Diff against target: 41 lines (+16/-4)
2 files modified
NEWS.rst (+7/-0)
rabbitfixture/server.py (+9/-4)
To merge this branch: bzr merge lp://staging/~cjwatson/rabbitfixture/signal-esrch
Reviewer Review Type Date Requested Status
Jürgen Gmach Approve
Review via email: mp+427306@code.staging.launchpad.net

Commit message

Ignore ESRCH errors in RabbitServerRunner._signal.

Description of the change

This can happen if the server process exits by itself just before we try to signal it.

To post a comment you must log in.
Revision history for this message
Jürgen Gmach (jugmac00) wrote :

How did you come up with this solution?

review: Approve
Revision history for this message
Colin Watson (cjwatson) wrote :

I saw http://lpbuildbot.canonical.com/builders/lp-db-devel-xenial/builds/2297/steps/shell/logs/summary, which wasn't quite the exact failure mode I remembered noticing before, and asked myself "how could that happen?". Since the code immediately preceding that was a check for whether the process is still running, the obvious answer was that it must be a race: the process exited by itself between the check for whether it was still running and the attempt to send a signal to it.

The private `_signal` method is only used to send various signals to the process that are intended to terminate it, either cooperatively (`SIGTERM`) or abruptly (`SIGKILL`). Therefore, it is not a problem if the process has already gone away, and the right answer is to simply ignore errors that mean that.

Finally, since `rabbitfixture` hasn't yet dropped Python 2 support and I didn't want to block a fix for this bug on that, I looked up https://docs.python.org/3/library/exceptions.html to confirm that `ProcessLookupError` corresponds to `ESRCH` and used the old approach of catching `OSError` and filtering by `errno` rather than catching `ProcessLookupError` directly, since that's only two more lines of code.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: