Address some review comments

There's already a nice way to reap a process if it's exited while not
waiting, and it doesn't require timeouts.

Change-Id: Ie327fecb6a3055ff146a94e1171ec0ec68d7179f
Related-Change: If6dc7b003e18ab4e8a5ed687c965025ebd417dfa

swift/obj/replicator.py

@@ -233,13 +233,10 @@ class ObjectReplicator(Daemon):
 
             reaped_procs = set()
             for proc in procs:
-                try:
+                # this will reap the process if it has exited, but
-                    # this will reap the process if it has exited, but
+                # otherwise will not wait
-                    # otherwise will not wait
+                if proc.poll() is not None:
-                    proc.wait(timeout=0)
                     reaped_procs.add(proc)
-                except subprocess.TimeoutExpired:
-                    pass
             procs -= reaped_procs
 
     def get_worker_args(self, once=False, **kwargs):

test/unit/obj/test_replicator.py

@@ -133,15 +133,15 @@ def _mock_process(ret):
 
 
 class MockHungProcess(object):
-    def __init__(self, waits_needed=1, *args, **kwargs):
+    def __init__(self, polls_needed=0, *args, **kwargs):
         class MockStdout(object):
             def read(self):
                 pass
         self.stdout = MockStdout()
         self._state = 'running'
         self._calls = []
-        self._waits = 0
+        self._polls = 0
-        self._waits_needed = waits_needed
+        self._polls_needed = polls_needed
 
     def wait(self, timeout=None):
         self._calls.append(('wait', self._state))
@@ -149,12 +149,22 @@ class MockHungProcess(object):
             # Sleep so we trip the rsync timeout
             sleep(1)
             raise BaseException('You need to mock out some timeouts')
-        elif self._state == 'killed':
+        if not self._polls_needed:
-            self._waits += 1
+            self._state = 'os-reaped'
-            if self._waits >= self._waits_needed:
+            return 137
-                return
+        if timeout is not None:
-            else:
+            raise subprocess.TimeoutExpired('some cmd', timeout)
-                raise subprocess.TimeoutExpired('some cmd', timeout)
+        raise BaseException("You're waiting indefinitely on something "
+                            "we've established is hung")
+
+    def poll(self):
+        self._calls.append(('poll', self._state))
+        self._polls += 1
+        if self._polls >= self._polls_needed:
+            self._state = 'os-reaped'
+            return 137
+        else:
+            return None
 
     def terminate(self):
         self._calls.append(('terminate', self._state))
@@ -2102,7 +2112,7 @@ class TestObjectReplicator(unittest.TestCase):
         mock_procs = []
 
         def new_mock(*a, **kw):
-            proc = MockHungProcess(waits_needed=2)
+            proc = MockHungProcess(polls_needed=2)
             mock_procs.append(proc)
             return proc
 
@@ -2116,7 +2126,8 @@ class TestObjectReplicator(unittest.TestCase):
                 ('wait', 'running'),
                 ('kill', 'running'),
                 ('wait', 'killed'),
-                ('wait', 'killed'),
+                ('poll', 'killed'),
+                ('poll', 'killed'),
             ])
         self.assertEqual(len(mock_procs), 2)