Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Fix empty url_cache_thumbnails/yyyy-mm-dd/ directories being left behind #10924

Merged
merged 6 commits into from
Sep 29, 2021
Merged

Fix empty url_cache_thumbnails/yyyy-mm-dd/ directories being left behind #10924

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d387b8e
Catch `FileNotFoundError`s instead of errnos in `_expire_url_cache_data`
Sep 27, 2021
ed23655
Extract 2 day expiry threshold into a constant
Sep 27, 2021
02d6f54
Fix empty `url_cache_thumbnails/yyyy-mm-dd/` directories being left b…
Sep 27, 2021
ecd5148
Add test for URL cache expiry
Sep 27, 2021
64733e9
Add newsfile
Sep 27, 2021
01492d6
Log failure to remove directories
Sep 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix empty url_cache_thumbnails/yyyy-mm-dd/ directories being left b… 
…ehind
  • Loading branch information
Sean Quah committed Sep 27, 2021
commit 02d6f54abffa9d0e2910d5ba67e5ea4d5b77d336
38 changes: 23 additions & 15 deletions synapse/rest/media/v1/preview_url_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,12 +512,14 @@ async def _expire_url_cache_data(self) -> None:

removed_media.append(media_id)

try:
dirs = self.filepaths.url_cache_filepath_dirs_to_delete(media_id)
for dir in dirs:
dirs = self.filepaths.url_cache_filepath_dirs_to_delete(media_id)
for dir in dirs:
try:
os.rmdir(dir)
except Exception:
pass
except FileNotFoundError:
pass # Already deleted, continue with deleting the rest
except Exception:
break # Failed, skip deleting the rest of the parent dirs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably log something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea. I got tired of the duplicated logic and factored it out into a function.


await self.store.delete_url_cache(removed_media)

Expand All @@ -544,12 +546,14 @@ async def _expire_url_cache_data(self) -> None:
logger.warning("Failed to remove media: %r: %s", media_id, e)
continue

try:
dirs = self.filepaths.url_cache_filepath_dirs_to_delete(media_id)
for dir in dirs:
dirs = self.filepaths.url_cache_filepath_dirs_to_delete(media_id)
for dir in dirs:
try:
os.rmdir(dir)
except Exception:
pass
except FileNotFoundError:
pass # Already deleted, continue with deleting the rest
except Exception:
break # Failed, skip deleting the rest of the parent dirs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here?


thumbnail_dir = self.filepaths.url_cache_thumbnail_directory(media_id)
try:
Expand All @@ -562,12 +566,16 @@ async def _expire_url_cache_data(self) -> None:

removed_media.append(media_id)

try:
dirs = self.filepaths.url_cache_thumbnail_dirs_to_delete(media_id)
for dir in dirs:
dirs = self.filepaths.url_cache_thumbnail_dirs_to_delete(media_id)
# Note that one of the directories to be deleted has already been
# removed by the `rmtree` above.
for dir in dirs:
try:
os.rmdir(dir)
except Exception:
pass
except FileNotFoundError:
pass # Already deleted, continue with deleting the rest
except Exception:
break # Failed, skip deleting the rest of the parent dirs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here?


await self.store.delete_url_cache_media(removed_media)

Expand Down