Kill subprocesses on KeyboardInterrupt.

SVN traps SIGINT and attempts to clean itself up, but this results in hangs
waiting for TCP. This patch does two things: daemonizes worker threads so they
are culled when the main thread dies (is ctrl-C'd) and keeps track of spawned
subprocesses to kill any remaining ones when the main program is ctrl-C'd.

A user ctrl-C'ing gclient has to manually terminate hung SVN processes, so this
introduces no extra data loss or hazard. stracing a hung SVN process shows that
it is indeed hanging on TCP reads after receiving a SIGINT, implying there is an
underlying but in the SVN binary.

Review URL: https://chromiumcodereview.appspot.com/14759006

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@198205 0039d316-1c4b-4281-b951-d872f2087c98

gclient.py

@@ -1777,6 +1777,9 @@ def Main(argv):
     # Not a known command. Default to help.
     GenUsage(parser, 'help')
     return CMDhelp(parser, argv)
+  except KeyboardInterrupt:
+    gclient_utils.GClientChildren.KillAllRemainingChildren()
+    raise
   except (gclient_utils.Error, subprocess2.CalledProcessError), e:
     print >> sys.stderr, 'Error: %s' % str(e)
     return 1

gclient_utils.py

@@ -323,6 +323,56 @@ def MakeFileAnnotated(fileobj, include_zero=False):
   return Annotated(fileobj)
 
 
+GCLIENT_CHILDREN = []
+GCLIENT_CHILDREN_LOCK = threading.Lock()
+
+
+class GClientChildren(object):
+  @staticmethod
+  def add(popen_obj):
+    with GCLIENT_CHILDREN_LOCK:
+      GCLIENT_CHILDREN.append(popen_obj)
+
+  @staticmethod
+  def remove(popen_obj):
+    with GCLIENT_CHILDREN_LOCK:
+      GCLIENT_CHILDREN.remove(popen_obj)
+
+  @staticmethod
+  def _attemptToKillChildren():
+    global GCLIENT_CHILDREN
+    with GCLIENT_CHILDREN_LOCK:
+      zombies = [c for c in GCLIENT_CHILDREN if c.poll() is None]
+
+    for zombie in zombies:
+      try:
+        zombie.kill()
+      except OSError:
+        pass
+
+    with GCLIENT_CHILDREN_LOCK:
+      GCLIENT_CHILDREN = [k for k in GCLIENT_CHILDREN if k.poll() is not None]
+
+  @staticmethod
+  def _areZombies():
+    with GCLIENT_CHILDREN_LOCK:
+      return bool(GCLIENT_CHILDREN)
+
+  @staticmethod
+  def KillAllRemainingChildren():
+    GClientChildren._attemptToKillChildren()
+
+    if GClientChildren._areZombies():
+      time.sleep(0.5)
+      GClientChildren._attemptToKillChildren()
+
+    with GCLIENT_CHILDREN_LOCK:
+      if GCLIENT_CHILDREN:
+        print >> sys.stderr, 'Could not kill the following subprocesses:'
+        for zombie in GCLIENT_CHILDREN:
+          print >> sys.stderr, '  ', zombie.pid
+
+
 def CheckCallAndFilter(args, stdout=None, filter_fn=None,
                        print_stdout=None, call_filter_on_first_line=False,
                        **kwargs):
@@ -344,6 +394,8 @@ def CheckCallAndFilter(args, stdout=None, filter_fn=None,
       args, bufsize=0, stdout=subprocess2.PIPE, stderr=subprocess2.STDOUT,
       **kwargs)
 
+  GClientChildren.add(kid)
+
   # Do a flush of stdout before we begin reading from the subprocess2's stdout
   stdout.flush()
 
@@ -375,6 +427,11 @@ def CheckCallAndFilter(args, stdout=None, filter_fn=None,
       if len(in_line):
         filter_fn(in_line)
     rv = kid.wait()
+
+    # Don't put this in a 'finally,' since the child may still run if we get an
+    # exception.
+    GClientChildren.remove(kid)
+
   except KeyboardInterrupt:
     print >> sys.stderr, 'Failed while running "%s"' % ' '.join(args)
     raise
@@ -657,6 +714,7 @@ class ExecutionQueue(object):
       self.index = index
       self.args = args
       self.kwargs = kwargs
+      self.daemon = True
 
     def run(self):
       """Runs in its own thread."""
@@ -664,18 +722,23 @@ class ExecutionQueue(object):
       work_queue = self.kwargs['work_queue']
       try:
         self.item.run(*self.args, **self.kwargs)
+      except KeyboardInterrupt:
+        logging.info('Caught KeyboardInterrupt in thread %s' % self.item.name)
+        logging.info(str(sys.exc_info()))
+        work_queue.exceptions.put(sys.exc_info())
+        raise
       except Exception:
         # Catch exception location.
         logging.info('Caught exception in thread %s' % self.item.name)
         logging.info(str(sys.exc_info()))
         work_queue.exceptions.put(sys.exc_info())
-      logging.info('_Worker.run(%s) done' % self.item.name)
-
-      work_queue.ready_cond.acquire()
-      try:
-        work_queue.ready_cond.notifyAll()
       finally:
-        work_queue.ready_cond.release()
+        logging.info('_Worker.run(%s) done' % self.item.name)
+        work_queue.ready_cond.acquire()
+        try:
+          work_queue.ready_cond.notifyAll()
+        finally:
+          work_queue.ready_cond.release()
 
 
 def GetEditor(git, git_editor=None):

tests/gclient_utils_test.py

@@ -34,7 +34,8 @@ class GclientUtilsUnittest(GclientUtilBase):
         'GetGClientRootAndEntries', 'GetEditor', 'IsDateRevision',
         'MakeDateRevision', 'MakeFileAutoFlush', 'MakeFileAnnotated',
         'PathDifference', 'ParseCodereviewSettingsContent', 'NumLocalCpus',
-        'PrintableObject', 'RunEditor',
+        'PrintableObject', 'RunEditor', 'GCLIENT_CHILDREN',
+        'GCLIENT_CHILDREN_LOCK', 'GClientChildren',
         'SplitUrlRevision', 'SyntaxErrorToError', 'UpgradeToHttps', 'Wrapper',
         'WorkItem', 'codecs', 'lockedmethod', 'logging', 'os', 'Queue', 're',
         'rmtree', 'safe_makedirs', 'stat', 'subprocess', 'subprocess2', 'sys',
@@ -49,6 +50,7 @@ class CheckCallAndFilterTestCase(GclientUtilBase):
   class ProcessIdMock(object):
     def __init__(self, test_string):
       self.stdout = StringIO.StringIO(test_string)
+      self.pid = 9284
     def wait(self):
       pass