diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 63e33a6fb4f..6b61ef2251b 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -161,14 +161,34 @@ def _log(self, msg): msg=msg)) -class SyncRepositoryTask(SyncRepositoryMixin, Task): +# 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) name = __name__ + '.sync_repository' + def run(self, *args, **kwargs): + step = SyncRepositoryTaskStep() + return step.run(*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. @@ -194,7 +214,20 @@ def run(self, version_pk): # pylint: disable=arguments-differ return False -class UpdateDocsTask(SyncRepositoryMixin, Task): +# 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 + 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. @@ -202,13 +235,16 @@ class UpdateDocsTask(SyncRepositoryMixin, Task): 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. - """ - max_retries = 5 - default_retry_delay = (7 * 60) - name = __name__ + '.update_docs' + .. 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. + + """ - # 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