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
--- 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': [