From 2fd6c3fcb7f33e2778fb74050b7a6ece711818e9 Mon Sep 17 00:00:00 2001 From: "xusydoc@chromium.org" Date: Fri, 3 May 2013 21:57:55 +0000 Subject: [PATCH] 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 | 3 ++ gclient_utils.py | 75 ++++++++++++++++++++++++++++++++++--- tests/gclient_utils_test.py | 4 +- 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/gclient.py b/gclient.py index 90b75d9a2..242cf0f2f 100755 --- a/gclient.py +++ b/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 diff --git a/gclient_utils.py b/gclient_utils.py index 041813b0d..bf9803e86 100644 --- a/gclient_utils.py +++ b/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): diff --git a/tests/gclient_utils_test.py b/tests/gclient_utils_test.py index 2703e5d95..4976d9a7b 100755 --- a/tests/gclient_utils_test.py +++ b/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