From af2d92200b25914b454aa0e125c7bf290145bd59 Mon Sep 17 00:00:00 2001 From: Anthony Johnson Date: Thu, 12 Apr 2018 23:12:21 -0600 Subject: [PATCH 1/2] Don't share state in build task Move task steps to an isolated class, instead of possibly sharing state in between tasks. This bug is not consistently reproduceable, but one consistent way to reproduce is to drop to ``rdb`` on the UpdateDocsTask run method. Previously, if you did this while running 2 celery processes, and triggered 2 builds, you could witness state changing on the task instance. By moving state to a separate class that is instantiated on each task execution, this completely avoids shared state in these tasks. --- readthedocs/projects/tasks.py | 27 ++++++++++++++++------ readthedocs/rtd_tests/tests/test_builds.py | 14 +++++------ readthedocs/rtd_tests/tests/test_celery.py | 18 +++++++-------- 3 files changed, 36 insertions(+), 23 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 63e33a6fb4f..ec0f7675658 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -161,7 +161,7 @@ def _log(self, msg): msg=msg)) -class SyncRepositoryTask(SyncRepositoryMixin, Task): +class SyncRepositoryTask(Task): """Entry point to synchronize the VCS documentation.""" @@ -169,6 +169,13 @@ class SyncRepositoryTask(SyncRepositoryMixin, Task): default_retry_delay = (7 * 60) name = __name__ + '.sync_repository' + def run(self, *args, **kwargs): + step = SyncRepositoryTaskStep() + return step.run(*args, **kwargs) + + +class SyncRepositoryTaskStep(SyncRepositoryMixin): + def run(self, version_pk): # pylint: disable=arguments-differ """ Run the VCS synchronization. @@ -194,7 +201,18 @@ def run(self, version_pk): # pylint: disable=arguments-differ return False -class UpdateDocsTask(SyncRepositoryMixin, Task): +class UpdateDocsTask(Task): + + max_retries = 5 + default_retry_delay = (7 * 60) + name = __name__ + '.update_docs' + + def run(self, *args, **kwargs): + step = UpdateDocsTaskStep() + return step.run(*args, **kwargs) + + +class UpdateDocsTaskStep(SyncRepositoryMixin): """ The main entry point for updating documentation. @@ -204,11 +222,6 @@ class UpdateDocsTask(SyncRepositoryMixin, Task): html docs if needed. """ - max_retries = 5 - default_retry_delay = (7 * 60) - name = __name__ + '.update_docs' - - # TODO: the argument from the __init__ are used only in tests def __init__(self, build_env=None, python_env=None, config=None, force=False, search=True, localmedia=True, build=None, project=None, version=None): diff --git a/readthedocs/rtd_tests/tests/test_builds.py b/readthedocs/rtd_tests/tests/test_builds.py index b53cd80b7ad..455d0bc410d 100644 --- a/readthedocs/rtd_tests/tests/test_builds.py +++ b/readthedocs/rtd_tests/tests/test_builds.py @@ -12,7 +12,7 @@ from readthedocs.doc_builder.environments import LocalBuildEnvironment from readthedocs.doc_builder.python_environments import Virtualenv from readthedocs.doc_builder.loader import get_builder_class -from readthedocs.projects.tasks import UpdateDocsTask +from readthedocs.projects.tasks import UpdateDocsTaskStep from readthedocs.rtd_tests.tests.test_config_wrapper import create_load from ..mocks.environment import EnvironmentMockGroup @@ -44,7 +44,7 @@ def test_build(self): build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load()()[0]) - task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, + task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env, version=version, search=False, localmedia=False, config=config) task.build_docs() @@ -68,7 +68,7 @@ def test_build_respects_pdf_flag(self): build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load()()[0]) - task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, + task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env, version=version, search=False, localmedia=False, config=config) task.build_docs() @@ -93,7 +93,7 @@ def test_build_respects_epub_flag(self): build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load()()[0]) - task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, + task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env, version=version, search=False, localmedia=False, config=config) task.build_docs() @@ -119,7 +119,7 @@ def test_build_respects_yaml(self): config = ConfigWrapper(version=version, yaml_config=create_load({ 'formats': ['epub'] })()[0]) - task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, + task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env, version=version, search=False, localmedia=False, config=config) task.build_docs() @@ -174,7 +174,7 @@ def test_build_pdf_latex_failures(self): build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load()()[0]) - task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, + task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env, version=version, search=False, localmedia=False, config=config) # Mock out the separate calls to Popen using an iterable side_effect @@ -216,7 +216,7 @@ def test_build_pdf_latex_not_failure(self): build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load()()[0]) - task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, + task = UpdateDocsTaskStep(build_env=build_env, project=project, python_env=python_env, version=version, search=False, localmedia=False, config=config) # Mock out the separate calls to Popen using an iterable side_effect diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index e9124baf104..f5aceb9519f 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -64,9 +64,9 @@ def test_clear_artifacts(self): self.assertTrue(result.successful()) self.assertFalse(exists(directory)) - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_python_environment', new=MagicMock) - @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', new=MagicMock) - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_python_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.build_docs', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_vcs', new=MagicMock) def test_update_docs(self): build = get(Build, project=self.project, version=self.project.versions.first()) @@ -79,10 +79,10 @@ def test_update_docs(self): intersphinx=False) self.assertTrue(result.successful()) - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_python_environment', new=MagicMock) - @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_python_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.build_docs', new=MagicMock) @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build', new=MagicMock) - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs') + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_vcs') def test_update_docs_unexpected_setup_exception(self, mock_setup_vcs): exc = Exception() mock_setup_vcs.side_effect = exc @@ -97,10 +97,10 @@ def test_update_docs_unexpected_setup_exception(self, mock_setup_vcs): intersphinx=False) self.assertTrue(result.successful()) - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_python_environment', new=MagicMock) - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_python_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_vcs', new=MagicMock) @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build', new=MagicMock) - @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs') + @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.build_docs') def test_update_docs_unexpected_build_exception(self, mock_build_docs): exc = Exception() mock_build_docs.side_effect = exc From 35145ecedc1f963738bb9401b1b8b22acad091dd Mon Sep 17 00:00:00 2001 From: Anthony Johnson Date: Wed, 18 Apr 2018 09:23:20 -0600 Subject: [PATCH 2/2] Add docs --- readthedocs/projects/tasks.py | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index ec0f7675658..6b61ef2251b 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -161,9 +161,11 @@ def _log(self, msg): msg=msg)) +# TODO SyncRepositoryTask should be refactored into a standard celery task, +# there is no more need to have this be a separate class class SyncRepositoryTask(Task): - """Entry point to synchronize the VCS documentation.""" + """Celery task to trigger VCS version sync.""" max_retries = 5 default_retry_delay = (7 * 60) @@ -176,6 +178,17 @@ def run(self, *args, **kwargs): class SyncRepositoryTaskStep(SyncRepositoryMixin): + """Entry point to synchronize the VCS documentation. + + .. note:: + + This is implemented as a separate class to isolate each run of the + underlying task. Previously, we were using a custom ``celery.Task`` for + this, but this class is only instantiated once -- on startup. The effect + was that this instance shared state between workers. + + """ + def run(self, version_pk): # pylint: disable=arguments-differ """ Run the VCS synchronization. @@ -201,6 +214,8 @@ def run(self, version_pk): # pylint: disable=arguments-differ return False +# TODO UpdateDocsTask should be refactored into a standard celery task, +# there is no more need to have this be a separate class class UpdateDocsTask(Task): max_retries = 5 @@ -220,6 +235,14 @@ class UpdateDocsTaskStep(SyncRepositoryMixin): It handles all of the logic around whether a project is imported, we created it or a webhook is received. Then it will sync the repository and build the html docs if needed. + + .. note:: + + This is implemented as a separate class to isolate each run of the + underlying task. Previously, we were using a custom ``celery.Task`` for + this, but this class is only instantiated once -- on startup. The effect + was that this instance shared state between workers. + """ def __init__(self, build_env=None, python_env=None, config=None,