Bug 1273556 - [mozlint] Better SIGINT handling, return partial results on Ctrl-C, r?jgraham draft
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Tue, 17 May 2016 12:24:42 -0400
changeset 367953 63d856a6261b19c911c098dba6ef81d6e9d06d02
parent 367830 0e6dbc3c06e64aaa206f52af29bb67dd9b12d801
child 521145 fa7d2fba5aa374fe97716cffee456b93aa9f30d1
push id18393
push userahalberstadt@mozilla.com
push dateTue, 17 May 2016 19:17:13 +0000
reviewersjgraham
bugs1273556
milestone49.0a1
Bug 1273556 - [mozlint] Better SIGINT handling, return partial results on Ctrl-C, r?jgraham Currently a bug in python (https://bugs.python.org/issue8296) is preventing a KeyboardInterrupt from reaching the parent process, meaning we can't kill the process with SIGINT. There is a workaround to this bug, but instead I decided to ignore SIGINT in the parent process completely. Now, each child process is responsible for handling SIGINT on its own. Since child processes should all shutdown relatively quickly anyway, this effectively also ends the parent process. The benefit of doing it this way is that each child process can return the results they have collected to date. So when a developer hits Ctrl-C, they'll still see some (but not all) formatted lint output. The downside is that a poorly implemented external linter could block the parent process from exiting quickly, but if this happens we should just fix the linter. MozReview-Commit-ID: 2tRJgtmoPYP
python/mozlint/mozlint/roller.py
python/mozlint/mozlint/types.py
tools/lint/flake8.lint
--- a/python/mozlint/mozlint/roller.py
+++ b/python/mozlint/mozlint/roller.py
@@ -20,18 +20,23 @@ from .parser import Parser
 
 
 def _run_linters(queue, paths, **lintargs):
     parse = Parser()
     results = defaultdict(list)
 
     while True:
         try:
+            # The astute reader may wonder what is preventing the worker from
+            # grabbing the next linter from the queue after a SIGINT. Because
+            # this is a Manager.Queue(), it is itself in a child process which
+            # also received SIGINT. By the time the worker gets back here, the
+            # Queue is dead and IOError is raised.
             linter_path = queue.get(False)
-        except Empty:
+        except (Empty, IOError):
             return results
 
         # Ideally we would pass the entire LINTER definition as an argument
         # to the worker instead of re-parsing it. But passing a function from
         # a dynamically created module (with imp) does not seem to be possible
         # with multiprocessing on Windows.
         linter = parse(linter_path)
         func = supported_types[linter['type']]
@@ -42,17 +47,19 @@ def _run_linters(queue, paths, **lintarg
 
         for r in res:
             results[r.path].append(r)
 
 
 def _run_worker(*args, **lintargs):
     try:
         return _run_linters(*args, **lintargs)
-    except:
+    except Exception:
+        # multiprocessing seems to munge worker exceptions, print
+        # it here so it isn't lost.
         traceback.print_exc()
         raise
 
 
 class LintRoller(object):
     """Registers and runs linters.
 
     :param lintargs: Arguments to pass to the underlying linter(s).
@@ -91,26 +98,26 @@ class LintRoller(object):
         m = Manager()
         queue = m.Queue()
 
         for linter in self.linters:
             queue.put(linter['path'])
 
         num_procs = num_procs or cpu_count()
         num_procs = min(num_procs, len(self.linters))
-
-        # ensure child processes ignore SIGINT so it reaches parent
-        orig = signal.signal(signal.SIGINT, signal.SIG_IGN)
         pool = Pool(num_procs)
-        signal.signal(signal.SIGINT, orig)
 
         all_results = defaultdict(list)
-        results = []
+        workers = []
         for i in range(num_procs):
-            results.append(
+            workers.append(
                 pool.apply_async(_run_worker, args=(queue, paths), kwds=self.lintargs))
+        pool.close()
 
-        for res in results:
-            # parent process blocks on res.get()
-            for k, v in res.get().iteritems():
+        # ignore SIGINT in parent so we can still get partial results
+        # from child processes. These should shutdown quickly anyway.
+        signal.signal(signal.SIGINT, signal.SIG_IGN)
+        for worker in workers:
+            # parent process blocks on worker.get()
+            for k, v in worker.get().iteritems():
                 all_results[k].extend(v)
 
         return all_results
--- a/python/mozlint/mozlint/types.py
+++ b/python/mozlint/mozlint/types.py
@@ -33,20 +33,23 @@ class BaseType(object):
             print("{}: No files to lint for specified paths!".format(linter['name']))
             return
 
         lintargs['exclude'] = exclude
         if self.batch:
             return self._lint(paths, linter, **lintargs)
 
         errors = []
-        for p in paths:
-            result = self._lint(p, linter, **lintargs)
-            if result:
-                errors.extend(result)
+        try:
+            for p in paths:
+                result = self._lint(p, linter, **lintargs)
+                if result:
+                    errors.extend(result)
+        except KeyboardInterrupt:
+            pass
         return errors
 
     @abstractmethod
     def _lint(self, path):
         pass
 
 
 class LineType(BaseType):
--- a/tools/lint/flake8.lint
+++ b/tools/lint/flake8.lint
@@ -1,17 +1,20 @@
 # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
 # vim: set filetype=python:
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import json
 import os
-import subprocess
+import signal
+
+import which
+from mozprocess import ProcessHandler
 
 from mozlint import result
 
 
 FLAKE8_NOT_FOUND = """
 Could not find flake8! Install flake8 and try again.
 
     $ pip install flake8
@@ -37,20 +40,36 @@ LINE_OFFSETS = {
     'E302': (-2, 3),
 }
 """Maps a flake8 error to a lineoffset tuple.
 
 The offset is of the form (lineno_offset, num_lines) and is passed
 to the lineoffset property of `ResultContainer`.
 """
 
+results = []
+
+
+def process_line(line):
+    try:
+        res = json.loads(line)
+    except ValueError:
+        return
+
+    if 'code' in res:
+        if res['code'].startswith('W'):
+            res['level'] = 'warning'
+
+        if res['code'] in LINE_OFFSETS:
+            res['lineoffset'] = LINE_OFFSETS[res['code']]
+
+    results.append(result.from_linter(LINTER, **res))
+
 
 def lint(files, **lintargs):
-    import which
-
     binary = os.environ.get('FLAKE8')
     if not binary:
         try:
             binary = which.which('flake8')
         except which.WhichError:
             pass
 
     if not binary:
@@ -64,37 +83,28 @@ def lint(files, **lintargs):
     ]
 
     exclude = lintargs.get('exclude')
     if exclude:
         cmdargs += ['--exclude', ','.join(lintargs['exclude'])]
 
     cmdargs += files
 
-    proc = subprocess.Popen(cmdargs, stdout=subprocess.PIPE, env=os.environ)
-    output = proc.communicate()[0]
-
-    if not output:
-        return []
+    # flake8 seems to handle SIGINT poorly. Handle it here instead
+    # so we can kill the process without a cryptic traceback.
+    orig = signal.signal(signal.SIGINT, signal.SIG_IGN)
+    proc = ProcessHandler(cmdargs, env=os.environ,
+                          processOutputLine=process_line)
+    proc.run()
+    signal.signal(signal.SIGINT, orig)
 
-    results = []
-    for line in output.splitlines():
-        try:
-            res = json.loads(line)
-        except ValueError:
-            continue
-
-        if 'code' in res:
-            if res['code'].startswith('W'):
-                res['level'] = 'warning'
-
-            if res['code'] in LINE_OFFSETS:
-                res['lineoffset'] = LINE_OFFSETS[res['code']]
-
-        results.append(result.from_linter(LINTER, **res))
+    try:
+        proc.wait()
+    except KeyboardInterrupt:
+        proc.kill()
 
     return results
 
 
 LINTER = {
     'name': "flake8",
     'description': "Python linter",
     'include': [