FEATURE: extend duration allowed for download

FEATURE: extend duration allowed for download

Previously we would raise a warning in the logs if downloading a file (from s3) takes longer than 60 seconds.

At scale this happens reasonably frequently.

  1. Raised the duration to 3 minutes

  2. Pulled the resizing mutex out of the downloading mutex so we have less and clearer error logs

diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb
index e578353..654d60a 100644
--- a/app/models/optimized_image.rb
+++ b/app/models/optimized_image.rb
@@ -56,6 +56,15 @@ class OptimizedImage < ActiveRecord::Base
 
     return thumbnail if thumbnail
 
+    # create the thumbnail otherwise
+    original_path = Discourse.store.path_for(upload)
+
+    if original_path.blank?
+      # download is protected with a DistributedMutex
+      external_copy = Discourse.store.download(upload) rescue nil
+      original_path = external_copy.try(:path)
+    end
+
     lock(upload.id, width, height) do
       # may have been generated since we got the lock
       thumbnail = find_by(upload_id: upload.id, width: width, height: height)
@@ -63,13 +72,6 @@ class OptimizedImage < ActiveRecord::Base
       # return the previous thumbnail if any
       return thumbnail if thumbnail
 
-      # create the thumbnail otherwise
-      original_path = Discourse.store.path_for(upload)
-      if original_path.blank?
-        external_copy = Discourse.store.download(upload) rescue nil
-        original_path = external_copy.try(:path)
-      end
-
       if original_path.blank?
         Rails.logger.error("Could not find file in the store located at url: #{upload.url}")
       else
diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb
index 6307465..df85960 100644
--- a/lib/file_store/base_store.rb
+++ b/lib/file_store/base_store.rb
@@ -77,7 +77,7 @@ module FileStore
     end
 
     def download(upload)
-      DistributedMutex.synchronize("download_#{upload.sha1}") do
+      DistributedMutex.synchronize("download_#{upload.sha1}", validity: 3.minutes) do
         filename = "#{upload.sha1}#{File.extname(upload.original_filename)}"
         file = get_from_cache(filename)
 

GitHub sha: 0cbaa8d8

1 Like

I think this needs to be move as well.

2 Likes

Any updates on this one folks? I don’t like to leave things pending forever.

1 Like

we could move it but then we have to re-nest the whole thing, not against changing it though

I think this should be approved, a big code shuffle and re-nesting this thing is not worth the effort imo

1 Like