From 8282885197714859d0b495e59cb2c542aa6d9e7b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 25 Jun 2018 11:54:03 -0300 Subject: [PATCH] Use PATHs to call clear_artifacts Use a list of PATHs Instead of using a ``Version.pk`` when calling `clear_artifacts`. This is because in the ``Version.delete`` method, the Version object is removed immediately after broadcasting the ``clear_artifacts`` task and that could produce a race condition. With this change, we calculate all the PATHs needed _before_ broadcasting the task and _before_ deleting the object from the database. --- readthedocs/builds/models.py | 20 ++++++- readthedocs/projects/tasks.py | 62 ++++++++-------------- readthedocs/projects/views/private.py | 4 +- readthedocs/rtd_tests/tests/test_celery.py | 4 +- 4 files changed, 45 insertions(+), 45 deletions(-) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 4bb8c3554ef..969db804b5a 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -181,7 +181,7 @@ def save(self, *args, **kwargs): # pylint: disable=arguments-differ def delete(self, *args, **kwargs): # pylint: disable=arguments-differ from readthedocs.projects import tasks log.info('Removing files for version %s', self.slug) - broadcast(type='app', task=tasks.clear_artifacts, args=[self.pk]) + broadcast(type='app', task=tasks.clear_artifacts, args=[self.get_artifact_paths()]) broadcast( type='app', task=tasks.symlink_project, args=[self.project.pk]) super(Version, self).delete(*args, **kwargs) @@ -237,6 +237,24 @@ def get_build_path(self): return path return None + def get_artifact_paths(self): + """ + Return a list of all production artifacts/media path for this version. + + :rtype: list + """ + paths = [] + + for type_ in ('pdf', 'epub', 'htmlzip'): + paths.append( + self.project.get_production_media_path( + type_=type_, + version_slug=self.slug), + ) + paths.append(self.project.rtd_build_path(version=self.slug)) + + return paths + def clean_build_path(self): """ Clean build path for project version. diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 01f674265bc..1806586cba8 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -764,10 +764,21 @@ def sync_files(project_pk, version_pk, hostname=None, html=False, synchronization of build artifacts on each application instance. """ # Clean up unused artifacts + version = Version.objects.get(pk=version_pk) if not pdf: - clear_pdf_artifacts(version_pk) + remove_dir( + version.project.get_production_media_path( + type_='pdf', + version_slug=version.slug, + ), + ) if not epub: - clear_epub_artifacts(version_pk) + remove_dir( + version.project.get_production_media_path( + type_='epub', + version_slug=version.slug, + ), + ) # Sync files to the web servers move_files( @@ -777,7 +788,7 @@ def sync_files(project_pk, version_pk, hostname=None, html=False, localmedia=localmedia, search=search, pdf=pdf, - epub=epub + epub=epub, ) # Symlink project @@ -1158,44 +1169,15 @@ def remove_dir(path): @app.task() -def clear_artifacts(version_pk): - """Remove artifacts from the web servers.""" - version = Version.objects.get(pk=version_pk) - clear_pdf_artifacts(version) - clear_epub_artifacts(version) - clear_htmlzip_artifacts(version) - clear_html_artifacts(version) - - -@app.task() -def clear_pdf_artifacts(version): - if isinstance(version, int): - version = Version.objects.get(pk=version) - remove_dir(version.project.get_production_media_path( - type_='pdf', version_slug=version.slug)) - - -@app.task() -def clear_epub_artifacts(version): - if isinstance(version, int): - version = Version.objects.get(pk=version) - remove_dir(version.project.get_production_media_path( - type_='epub', version_slug=version.slug)) - - -@app.task() -def clear_htmlzip_artifacts(version): - if isinstance(version, int): - version = Version.objects.get(pk=version) - remove_dir(version.project.get_production_media_path( - type_='htmlzip', version_slug=version.slug)) - +def clear_artifacts(paths): + """ + Remove artifacts from the web servers. -@app.task() -def clear_html_artifacts(version): - if isinstance(version, int): - version = Version.objects.get(pk=version) - remove_dir(version.project.rtd_build_path(version=version.slug)) + :param paths: list containing PATHs where production media is on disk + (usually ``Version.get_artifact_paths``) + """ + for path in paths: + remove_dir(path) @app.task(queue='web') diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 1dc197096eb..30731529d8a 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -157,7 +157,7 @@ def project_version_detail(request, project_slug, version_slug): if 'active' in form.changed_data and version.active is False: log.info('Removing files for version %s', version.slug) broadcast( - type='app', task=tasks.clear_artifacts, args=[version.pk]) + type='app', task=tasks.clear_artifacts, args=[version.get_artifact_paths()]) version.built = False version.save() url = reverse('project_version_list', args=[project.slug]) @@ -649,7 +649,7 @@ def project_version_delete_html(request, project_slug, version_slug): if not version.active: version.built = False version.save() - broadcast(type='app', task=tasks.clear_artifacts, args=[version.pk]) + broadcast(type='app', task=tasks.clear_artifacts, args=[version.get_artifact_paths()]) else: return HttpResponseBadRequest( "Can't delete HTML for an active version.") diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index 1a8e9fc15d6..6d9cdac81bc 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -60,14 +60,14 @@ def test_clear_artifacts(self): directory = self.project.get_production_media_path(type_='pdf', version_slug=version.slug) os.makedirs(directory) self.assertTrue(exists(directory)) - result = tasks.clear_artifacts.delay(version_pk=version.pk) + result = tasks.clear_artifacts.delay(paths=version.get_artifact_paths()) self.assertTrue(result.successful()) self.assertFalse(exists(directory)) directory = version.project.rtd_build_path(version=version.slug) os.makedirs(directory) self.assertTrue(exists(directory)) - result = tasks.clear_artifacts.delay(version_pk=version.pk) + result = tasks.clear_artifacts.delay(paths=version.get_artifact_paths()) self.assertTrue(result.successful()) self.assertFalse(exists(directory))