AO3-5631 Make /tmp download directory unique per generation #3532
Conversation
Add the file type and current timestamp to the directory name. This means each generation has to stay with the same Download instance for proper cleanup, and the Download class methods become obsolete. Having timestamped directory names means we need to freeze time so the tests can find the generated files.
app/models/download.rb
Outdated
|
|
||
| def initialize(work, options = {}) | ||
| @work = work | ||
| @file_type = set_file_type(options.slice(:mime_type, :format)) | ||
| # TODO: Our current version of the mime-types gem doesn't include azw3, but | ||
| # the gem cannot be updated without updating rest-client | ||
| @mime_type = @file_type == "azw3" ? "application/x-mobi8-ebook" : MIME::Types.type_for(@file_type).first | ||
| @timestamp = Time.now.to_i |
There was a problem hiding this comment.
Is this unique enough or should I go for fractions of seconds as well?
(Time.now.to_f * 1000).to_i
There was a problem hiding this comment.
If you're concerned about timing issues, what about using Dir.mktmpdir? It uses random numbers and checks for EEXIST on mkdir.
There was a problem hiding this comment.
Nice, thanks! I'll use that. I need to rewrite the tests to not rely on file location checks since we won't know the directory name.
redsummernight
added
Coder Has Actioned Review
and removed
Reviewed: Action Needed
labels
| @@ -846,11 +846,6 @@ def set_word_count(preview = false) | |||
| end | |||
| end | |||
|
|
|||
| after_update :remove_outdated_downloads | |||
There was a problem hiding this comment.
Does this mean that more files will be left around ?
There was a problem hiding this comment.
No, because we delete them after we generate and send them off.
| @@ -47,7 +48,7 @@ def load_work | |||
| def remove_downloads | |||
| yield | |||
| ensure | |||
| Download.remove(@work) unless Rails.env.test? | |||
| @download.remove | |||
There was a problem hiding this comment.
Do we not want to delete the directory and things like the zip file ?
There was a problem hiding this comment.
We do, @download.remove does that.
There was a problem hiding this comment.
FileUtils.rm_rf(dir) is not subtle
Issue
https://otwarchive.atlassian.net/browse/AO3-5631
Purpose
Add the file type and current timestamp to the directory name. This means each generation has to stay with the same Download instance for proper cleanup, and the Download class methods become obsolete.
Having timestamped directory names means we need to freeze time so the tests can find the generated files.
Testing Instructions
Confirm downloads still work on staging.
Locally, I repeated the testing I did for #3528 as well.