misc fixes

* always use a timeout for assertDead
* don't try to kill a pgid if none is known (indentation error)
* only set process.pgid in connectionMade

slave/buildslave/runprocess.py

@@ -147,7 +147,6 @@ def __init__(self, command):
         self.pending_stdin = ""
         self.stdin_finished = False
         self.killed = False
-        self.pgid = None
 
     def setStdin(self, data):
         assert not self.connected
@@ -497,10 +496,6 @@ def _startCommand(self):
                                  self.workdir,
                                  usePTY=self.usePTY)
 
-        # keep the pgid for later, if using process groups
-        if self.useProcGroup:
-            self.process.pgid = self.process.pid
-
         # set up timeouts
 
         if self.timeout:
@@ -752,17 +747,17 @@ def kill(self, msg):
             else:
                 log.msg("trying to kill process group %d" %
                                                 (self.process.pgid,))
-            try:
-                os.kill(-self.process.pgid, sig)
-                log.msg(" signal %s sent successfully" % sig)
-                self.process.pgid = None
-                hit = 1
-            except OSError:
-                log.msg('failed to kill process group (ignored): %s' %
-                        (sys.exc_info()[1],))
-                # probably no-such-process, maybe because there is no process
-                # group
-                pass
+                try:
+                    os.kill(-self.process.pgid, sig)
+                    log.msg(" signal %s sent successfully" % sig)
+                    self.process.pgid = None
+                    hit = 1
+                except OSError:
+                    log.msg('failed to kill process group (ignored): %s' %
+                            (sys.exc_info()[1],))
+                    # probably no-such-process, maybe because there is no process
+                    # group
+                    pass
 
         # try signalling the process itself (works on Windows too, sorta)
         if not hit:

slave/buildslave/test/unit/test_runprocess.py

@@ -312,7 +312,7 @@ def assertAlive(self, pid):
         except OSError:
             self.fail("pid %d still alive" % (pid,))
 
-    def assertDead(self, pid, timeout=0):
+    def assertDead(self, pid, timeout=5):
         log.msg("checking pid %r" % (pid,))
         def check():
             try:
@@ -358,7 +358,7 @@ def check_alive(pid):
         pidfile_d.addCallback(check_alive)
 
         def check_dead(_):
-            self.assertRaises(OSError, lambda : os.kill(self.pid, 0))
+            self.assertDead(self.pid)
         runproc_d.addCallback(check_dead)
         return defer.gatherResults([pidfile_d, runproc_d])
 
@@ -371,7 +371,7 @@ def test_pgroup_no_usePTY(self):
     def test_pgroup_no_usePTY_no_pgroup(self):
         # note that this configuration is not *used*, but that it is
         # still supported, and correctly fails to kill the child process
-        return self.do_test_double_fork(usePTY=False, useProcGroup=False,
+        return self.do_test_pgroup(usePTY=False, useProcGroup=False,
                 expectChildSurvival=True)
 
     def do_test_pgroup(self, usePTY, useProcGroup=True,
@@ -408,7 +408,7 @@ def check_dead(_):
             if expectChildSurvival:
                 self.assertAlive(self.child_pid)
             else:
-                self.assertDead(self.child_pid, timeout=5)
+                self.assertDead(self.child_pid)
         d.addCallback(check_dead)
         return d
 
@@ -456,11 +456,11 @@ def kill(_):
         # check that both processes are dead after RunProcess is done
         d = defer.gatherResults([pidfiles_d, runproc_d])
         def check_dead(_):
-            self.assertDead(self.parent_pid, timeout=5)
+            self.assertDead(self.parent_pid)
             if expectChildSurvival:
                 self.assertAlive(self.child_pid)
             else:
-                self.assertDead(self.child_pid, timeout=5)
+                self.assertDead(self.child_pid)
         d.addCallback(check_dead)
         return d