diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index 55e4080a022..5919ccf13ee 100644 --- a/app/models/optimized_image.rb +++ b/app/models/optimized_image.rb @@ -38,18 +38,23 @@ class OptimizedImage < ActiveRecord::Base end end + # prefer to look up the thumbnail without grabbing any locks + thumbnail = find_by(upload_id: upload.id, width: width, height: height) + + # correct bad thumbnail if needed + if thumbnail && thumbnail.url.blank? + thumbnail.destroy + thumbnail = nil + end + + return thumbnail if thumbnail + lock(upload.id, width, height) do - # do we already have that thumbnail? + # may have been generated since we got the lock thumbnail = find_by(upload_id: upload.id, width: width, height: height) - # make sure we have an url - if thumbnail && thumbnail.url.blank? - thumbnail.destroy - thumbnail = nil - end - # return the previous thumbnail if any - return thumbnail unless thumbnail.nil? + return thumbnail if thumbnail # create the thumbnail otherwise original_path = Discourse.store.path_for(upload) diff --git a/lib/distributed_mutex.rb b/lib/distributed_mutex.rb index b014a554a32..76def3e7859 100644 --- a/lib/distributed_mutex.rb +++ b/lib/distributed_mutex.rb @@ -7,15 +7,26 @@ class DistributedMutex def initialize(key, redis = nil) @key = key + @using_global_redis = true if !redis @redis = redis || $redis @mutex = Mutex.new end + CHECK_READONLY_ATTEMPT ||= 10 + # NOTE wrapped in mutex to maintain its semantics def synchronize + @mutex.lock + attempts = 0 + while !try_to_get_lock sleep 0.001 + attempts += 1 + # in readonly we will never be able to get a lock + if @using_global_redis && attempts > CHECK_READONLY_ATTEMPT + raise Discourse::ReadOnly + end end yield diff --git a/spec/components/distributed_mutex_spec.rb b/spec/components/distributed_mutex_spec.rb index 69080c50512..3975ea0fc1e 100644 --- a/spec/components/distributed_mutex_spec.rb +++ b/spec/components/distributed_mutex_spec.rb @@ -45,4 +45,29 @@ describe DistributedMutex do }.to raise_error(ThreadError) end + context "readonly redis" do + before do + $redis.slaveof "127.0.0.1", "99991" + end + + after do + $redis.slaveof "no", "one" + end + + it "works even if redis is in readonly" do + m = DistributedMutex.new("test_readonly") + start = Time.now + done = false + + expect { + m.synchronize do + done = true + end + }.to raise_error(Discourse::ReadOnly) + + expect(done).to eq(false) + expect(Time.now - start).to be < (1.second) + end + end + end