From 1c6c1f51163784005dac358d60ecb8781d726151 Mon Sep 17 00:00:00 2001 From: Ian Cordasco Date: Wed, 19 Aug 2015 20:32:52 -0500 Subject: [PATCH 1/3] Refactor how we use StyleGuides for better error recovery In bug 74 we discovered that there are some less than ideal problems around our use of multiprocessing. This is a first attempt at fixing 74 by using a fake StyleGuide object which proxies to the real one, and will catch and handle exceptions and then posibly retry the operation we were trying to perform in the first place. Currently we're only implementing that logic for StyleGuide.check_files but we should be careful to implement this in other functions used in hooks and elsewhere. Note: there may be a simpler way to fix this with a context manager that will do the right thing. That may also prove simpler to implement but that will have a much larger impact on the code-base than this. Related to bug #74 --- flake8/engine.py | 64 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-) diff --git a/flake8/engine.py b/flake8/engine.py index d18cc2c..cb0702a 100644 --- a/flake8/engine.py +++ b/flake8/engine.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- -import re +import errno import platform +import re import warnings import pep8 @@ -81,7 +82,7 @@ def get_parser(): return parser, options_hooks -class StyleGuide(pep8.StyleGuide): +class NoQAStyleGuide(pep8.StyleGuide): def input_file(self, filename, lines=None, expected=None, line_offset=0): """Run all checks on a Python source file.""" @@ -95,6 +96,65 @@ class StyleGuide(pep8.StyleGuide): return fchecker.check_all(expected=expected, line_offset=line_offset) +class StyleGuide(object): + """A wrapper StyleGuide object for Flake8 usage. + + This allows for OSErrors to be caught in the styleguide and special logic + to be used to handle those errors. + """ + + # Reasoning for error numbers is in-line below + turn_off_multiprocessing_errors = set([ + # ENOSPC: Added by sigmavirus24 + # > On some operating systems (OSX), multiprocessing may cause an + # > ENOSPC error while trying to trying to create a Semaphore. + # > In those cases, we should replace the customized Queue Report + # > class with pep8's StandardReport class to ensure users don't run + # > into this problem. + # > (See also: https://gitlab.com/pycqa/flake8/issues/74) + errno.ENOSPC, + # NOTE(sigmavirus24): When adding to this list, include the reasoning + # on the lines before the error code and always append your error + # code. Further, please always add a trailing `,` to reduce the visual + # noise in diffs. + ]) + + def __init__(self, **kwargs): + self._styleguide = NoQAStyleGuide(**kwargs) + + @property + def options(self): + return self._styleguide.options + + @property + def paths(self): + return self._styleguide.paths + + def check_files(self, paths=None): + try: + return self._styleguide.check_files(paths) + except IOError as ioerr: + if ioerr.errno in self.turn_off_multiprocessing_errors: + self.init_report(pep8.StandardReport) + else: + raise + return self._styleguide.check_files(paths) + + def excluded(self, filename, parent=None): + return self._styleguide.excluded(filename, parent=parent) + + def init_report(self, reporter=None): + return self._styleguide.init_report(reporter) + + def input_file(self, filename, lines=None, expected=None, line_offset=0): + return self._styleguide.input_file( + filename=filename, + lines=lines, + expected=expected, + line_offset=0, + ) + + def _disable_extensions(parser, options): ignored_extensions = set(getattr(parser, 'ignored_extensions', [])) # Remove any of the selected extensions from the extensions ignored by From f1aa58889df0af538e5dc97047bc441edfe61a1d Mon Sep 17 00:00:00 2001 From: Ian Cordasco Date: Thu, 20 Aug 2015 21:13:18 -0500 Subject: [PATCH 2/3] Add a helper method for retrying checks in serial This allows us to reuse the same code simply for check_files and input_file. This should cover all uses of the StyleGuide methods. Related to bug #74 --- flake8/engine.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/flake8/engine.py b/flake8/engine.py index cb0702a..9cbdf6d 100644 --- a/flake8/engine.py +++ b/flake8/engine.py @@ -104,7 +104,7 @@ class StyleGuide(object): """ # Reasoning for error numbers is in-line below - turn_off_multiprocessing_errors = set([ + serial_retry_errors = set([ # ENOSPC: Added by sigmavirus24 # > On some operating systems (OSX), multiprocessing may cause an # > ENOSPC error while trying to trying to create a Semaphore. @@ -130,15 +130,24 @@ class StyleGuide(object): def paths(self): return self._styleguide.paths - def check_files(self, paths=None): + def _retry_serial(self, func, *args, **kwargs): + """This will retry the passed function in serial if necessary. + + In the event that we encounter an OSError with an errno in + :attr:`serial_retry_errors`, this function will retry this function + using pep8's default Report class which operates in serial. + """ try: - return self._styleguide.check_files(paths) + return func(*args, **kwargs) except IOError as ioerr: - if ioerr.errno in self.turn_off_multiprocessing_errors: + if ioerr.errno in self.serial_retry_errors: self.init_report(pep8.StandardReport) else: raise - return self._styleguide.check_files(paths) + return func(*args, **kwargs) + + def check_files(self, paths=None): + return self._retry_serial(self._styleguide.check_files, paths=paths) def excluded(self, filename, parent=None): return self._styleguide.excluded(filename, parent=parent) @@ -147,7 +156,8 @@ class StyleGuide(object): return self._styleguide.init_report(reporter) def input_file(self, filename, lines=None, expected=None, line_offset=0): - return self._styleguide.input_file( + return self._retry_serial( + self._styleguide.input_file, filename=filename, lines=lines, expected=expected, From 9d734158ca72c264810c6a4c2d22c3c3160864b1 Mon Sep 17 00:00:00 2001 From: Ian Cordasco Date: Sat, 22 Aug 2015 19:44:44 -0500 Subject: [PATCH 3/3] Add tests around the OSError retry logic This updates our retry logic to be more specific in catching OSErrors and it adds specific tests to show that it works and properly re-initializes the StyleGuide with the pep8.StandardReport class so we can fall back on serial behaviour gracefully. Closes #74 --- flake8/engine.py | 9 ++-- flake8/tests/test_engine.py | 84 +++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 4 deletions(-) diff --git a/flake8/engine.py b/flake8/engine.py index 9cbdf6d..0f440c1 100644 --- a/flake8/engine.py +++ b/flake8/engine.py @@ -120,7 +120,8 @@ class StyleGuide(object): ]) def __init__(self, **kwargs): - self._styleguide = NoQAStyleGuide(**kwargs) + # This allows us to inject a mocked StyleGuide in the tests. + self._styleguide = kwargs.pop('styleguide', NoQAStyleGuide(**kwargs)) @property def options(self): @@ -139,8 +140,8 @@ class StyleGuide(object): """ try: return func(*args, **kwargs) - except IOError as ioerr: - if ioerr.errno in self.serial_retry_errors: + except OSError as oserr: + if oserr.errno in self.serial_retry_errors: self.init_report(pep8.StandardReport) else: raise @@ -161,7 +162,7 @@ class StyleGuide(object): filename=filename, lines=lines, expected=expected, - line_offset=0, + line_offset=line_offset, ) diff --git a/flake8/tests/test_engine.py b/flake8/tests/test_engine.py index 60b388c..a6faab5 100644 --- a/flake8/tests/test_engine.py +++ b/flake8/tests/test_engine.py @@ -1,5 +1,6 @@ from __future__ import with_statement +import errno import unittest try: from unittest import mock @@ -7,6 +8,7 @@ except ImportError: import mock # < PY33 from flake8 import engine, util, __version__, reporter +import pep8 class TestEngine(unittest.TestCase): @@ -112,5 +114,87 @@ class TestEngine(unittest.TestCase): assert 'X' not in sg.options.ignore +def oserror_generator(error_number, message='Ominous OSError message'): + def oserror_side_effect(*args, **kwargs): + if hasattr(oserror_side_effect, 'used'): + return + + oserror_side_effect.used = True + raise OSError(error_number, message) + + return oserror_side_effect + + +class TestStyleGuide(unittest.TestCase): + def setUp(self): + mocked_styleguide = mock.Mock(spec=engine.NoQAStyleGuide) + self.styleguide = engine.StyleGuide(styleguide=mocked_styleguide) + self.mocked_sg = mocked_styleguide + + def test_proxies_excluded(self): + self.styleguide.excluded('file.py', parent='.') + + self.mocked_sg.excluded.assert_called_once_with('file.py', parent='.') + + def test_proxies_init_report(self): + reporter = object() + self.styleguide.init_report(reporter) + + self.mocked_sg.init_report.assert_called_once_with(reporter) + + def test_proxies_check_files(self): + self.styleguide.check_files(['foo', 'bar']) + + self.mocked_sg.check_files.assert_called_once_with( + paths=['foo', 'bar'] + ) + + def test_proxies_input_file(self): + self.styleguide.input_file('file.py', + lines=[9, 10], + expected='foo', + line_offset=20) + + self.mocked_sg.input_file.assert_called_once_with(filename='file.py', + lines=[9, 10], + expected='foo', + line_offset=20) + + def test_check_files_retries_on_specific_OSErrors(self): + self.mocked_sg.check_files.side_effect = oserror_generator( + errno.ENOSPC, 'No space left on device' + ) + + self.styleguide.check_files(['foo', 'bar']) + + self.mocked_sg.init_report.assert_called_once_with(pep8.StandardReport) + + def test_input_file_retries_on_specific_OSErrors(self): + self.mocked_sg.input_file.side_effect = oserror_generator( + errno.ENOSPC, 'No space left on device' + ) + + self.styleguide.input_file('file.py') + + self.mocked_sg.init_report.assert_called_once_with(pep8.StandardReport) + + def test_check_files_reraises_unknown_OSErrors(self): + self.mocked_sg.check_files.side_effect = oserror_generator( + errno.EADDRINUSE, + 'lol why are we talking about binding to sockets' + ) + + self.assertRaises(OSError, self.styleguide.check_files, + ['foo', 'bar']) + + def test_input_file_reraises_unknown_OSErrors(self): + self.mocked_sg.input_file.side_effect = oserror_generator( + errno.EADDRINUSE, + 'lol why are we talking about binding to sockets' + ) + + self.assertRaises(OSError, self.styleguide.input_file, + ['foo', 'bar']) + if __name__ == '__main__': unittest.main()