Merge lp:~cjwatson/rabbitfixture/work-around-stop-hang into lp:rabbitfixture

[Colin Watson]

Apply a timeout to all rabbitmqctl calls to work around occasional hangs on stop.

Apply a timeout to all rabbitmqctl calls to work around occasional hangs on stop.

subprocess32 is used because that adds a timeout facility to communicate().

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'rabbitfixture/server.py'
--- rabbitfixture/server.py	2014-05-29 12:26:57 +0000
+++ rabbitfixture/server.py	2015-04-24 12:33:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test server fixtures for RabbitMQ."""
@@ -14,7 +14,10 @@
 import re
 import signal
 import socket
-import subprocess
+try:
+    import subprocess32 as subprocess
+except ImportError:
+    import subprocess
 import time
 
 from amqplib import client_0_8 as amqp
@@ -194,18 +197,31 @@
                 yield error
             yield '\n'
 
-    def rabbitctl(self, command, strip=False):
+    @property
+    def ctlbin(self):
+        return os.path.join(RABBITBIN, "rabbitmqctl")
+
+    @property
+    def ctltimeout(self):
+        return 15
+
+    def rabbitctl(self, command, strip=False, timeout=None):
         """Executes a ``rabbitctl`` command and returns status."""
-        ctlbin = os.path.join(RABBITBIN, "rabbitmqctl")
+        if timeout is None:
+            timeout = self.ctltimeout
         nodename = self.config.fq_nodename
         env = dict(os.environ, HOME=self.config.homedir)
         if isinstance(command, str):
             command = (command,)
         ctl = subprocess.Popen(
-            (ctlbin, "-n", nodename) + command, env=env,
+            (self.ctlbin, "-n", nodename) + command, env=env,
             stdout=subprocess.PIPE, stderr=subprocess.PIPE,
             preexec_fn=preexec_fn)
-        outstr, errstr = ctl.communicate()
+        try:
+            outstr, errstr = ctl.communicate(timeout=timeout)
+        except subprocess.TimeoutExpired:
+            ctl.kill()
+            raise
         if strip:
             return outstr.strip(), errstr.strip()
         return outstr, errstr
@@ -213,7 +229,10 @@
     def is_node_running(self):
         """Checks that our RabbitMQ node is up and running."""
         nodename = self.config.fq_nodename
-        outdata, errdata = self.rabbitctl("status")
+        try:
+            outdata, errdata = self.rabbitctl("status")
+        except subprocess.TimeoutExpired:
+            return False
         if errdata:
             self._errors.append(errdata)
         if not outdata:
@@ -336,16 +355,21 @@
 
     def _stop(self):
         """Stop the running server. Normally called by cleanups."""
-        self._request_stop()
-        # Wait for the node to go down...
-        timeout = time.time() + 15
-        while time.time() < timeout:
-            if not self.environment.is_node_running():
-                break
-            time.sleep(0.3)
-        else:
-            raise Exception(
-                "Timeout waiting for RabbitMQ node to go down.")
+        try:
+            self._request_stop()
+            # Wait for the node to go down...
+            timeout = time.time() + 15
+            while time.time() < timeout:
+                if not self.environment.is_node_running():
+                    break
+                time.sleep(0.3)
+            else:
+                raise Exception(
+                    "Timeout waiting for RabbitMQ node to go down.")
+        except subprocess.TimeoutExpired:
+            # Go straight to killing the process directly.
+            timeout = time.time()
+
         # Wait at least 5 more seconds for the process to end...
         timeout = max(timeout, time.time() + 5)
         while time.time() < timeout:

=== modified file 'rabbitfixture/tests/test_server.py'
--- rabbitfixture/tests/test_server.py	2014-05-29 12:33:23 +0000
+++ rabbitfixture/tests/test_server.py	2015-04-24 12:33:24 +0000
@@ -7,10 +7,15 @@
 
 import os.path
 import socket
+import stat
 from textwrap import dedent
 
 from amqplib import client_0_8 as amqp
-from fixtures import EnvironmentVariableFixture
+from fixtures import (
+    EnvironmentVariableFixture,
+    MonkeyPatch,
+    TempDir,
+    )
 from rabbitfixture.server import (
     get_nodename_from_status,
     RabbitServer,
@@ -55,6 +60,40 @@
         # The daemon should be closed now.
         self.assertRaises(socket.error, amqp.Connection, **connect_arguments)
 
+    def test_stop_hang(self):
+        # If rabbitctl hangs on shutdown, the fixture eventually manages to
+        # stop RabbitMQ anyway.
+        bindir = self.useFixture(TempDir()).path
+        fakectlbin = os.path.join(bindir, "rabbitmqctl")
+        with open(fakectlbin, "w") as f:
+            f.write("#! /bin/sh\n")
+            f.write("while :; do sleep 1 || exit; done\n")
+        os.chmod(fakectlbin, stat.S_IRWXU)
+
+        with RabbitServer() as fixture:
+            try:
+                connect_arguments = {
+                    "host": 'localhost:%s' % fixture.config.port,
+                    "userid": "guest", "password": "guest",
+                    "virtual_host": "/", "insist": False,
+                    }
+
+                self.useFixture(MonkeyPatch(
+                    "rabbitfixture.server.RabbitServerEnvironment.ctlbin",
+                    fakectlbin))
+                self.useFixture(MonkeyPatch(
+                    "rabbitfixture.server.RabbitServerEnvironment.ctltimeout",
+                    0.1))
+            except Exception:
+                # self.useFixture() is not being used because we want to
+                # handle the fixture's lifecycle, so we must also be
+                # responsible for propagating fixture details.
+                gather_details(fixture.getDetails(), self.getDetails())
+                raise
+
+        # The daemon should be closed now.
+        self.assertRaises(socket.error, amqp.Connection, **connect_arguments)
+
     def test_config(self):
         # The configuration can be passed in.
         config = RabbitServerResources()

=== modified file 'setup.py'
--- setup.py	2014-05-29 13:19:05 +0000
+++ setup.py	2015-04-24 12:33:24 +0000
@@ -1,9 +1,20 @@
 #!/usr/bin/env python
 """Distutils installer for rabbitfixture."""
 
+import sys
+
 from setuptools import setup, find_packages
 
 
+install_requires = [
+    'amqplib >= 0.6.1',
+    'fixtures >= 0.3.6',
+    'setuptools',
+    'testtools >= 0.9.12',
+    ]
+if sys.version_info[0] < 3:
+    install_requires.append('subprocess32')
+
 setup(
     name='rabbitfixture',
     version="0.3.5",
@@ -12,9 +23,4 @@
     include_package_data=True,
     zip_safe=False,
     description='Magic.',
-    install_requires=[
-        'amqplib >= 0.6.1',
-        'fixtures >= 0.3.6',
-        'setuptools',
-        'testtools >= 0.9.12',
-        ])
+    install_requires=install_requires)