From 47e3f1883d7c9c47d0169a7e9f665d9090058879 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Sun, 26 Feb 2023 12:25:48 +0100 Subject: [PATCH 1/3] PictureUrl: Load from DB if thumbs not preloaded This makes sure we only load from the DB if necessary. Theoretically this would also happen with the former implementation, but ActiveStorage is doing this as well, so I can't hurt to do it explicilty. --- app/models/alchemy/picture/url.rb | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/app/models/alchemy/picture/url.rb b/app/models/alchemy/picture/url.rb index e33f14e3ad..c1423e5502 100644 --- a/app/models/alchemy/picture/url.rb +++ b/app/models/alchemy/picture/url.rb @@ -3,7 +3,7 @@ module Alchemy class Picture < BaseRecord class Url - attr_reader :variant + attr_reader :variant, :thumb # @param [Alchemy::PictureVariant] # @@ -31,14 +31,21 @@ def processible_image? def uid signature = PictureThumb::Signature.call(variant) - thumb = variant.picture.thumbs.detect { |t| t.signature == signature } - if thumb - uid = thumb.uid + if find_thumb_by(signature) + thumb.uid else uid = PictureThumb::Uid.call(signature, variant) PictureThumb.generator_class.call(variant, signature, uid) + uid end - uid + end + + def find_thumb_by(signature) + @thumb = if variant.picture.thumbs.loaded? + variant.picture.thumbs.find { |t| t.signature == signature } + else + variant.picture.thumbs.find_by(signature: signature) + end end end end From 611397418d7b9f15b6ffdf74944ab2ed82b78887 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Sun, 26 Feb 2023 12:36:05 +0100 Subject: [PATCH 2/3] Fix race condition in PictureThumb::Create In a multi concurrent application server (like Puma) it happens that a thumb might already exist for a given signature during the very short timeframe of finding it vs creating it. Using ARs [create_or_find_by!](https://github.com/rails/rails/blob/8015c2c2cf5c8718449677570f372ceb01318a32/activerecord/lib/active_record/relation.rb#L218) do avoid ActiveRecord::RecordNotUnique errors. ActiveStorage does the exact same thing to avoid concurrency issues. Closes #2429 --- app/models/alchemy/picture_thumb/create.rb | 11 +++++------ spec/models/alchemy/picture_thumb/create_spec.rb | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/app/models/alchemy/picture_thumb/create.rb b/app/models/alchemy/picture_thumb/create.rb index fff38f83ea..0334f4e4c3 100644 --- a/app/models/alchemy/picture_thumb/create.rb +++ b/app/models/alchemy/picture_thumb/create.rb @@ -19,11 +19,10 @@ def call(variant, signature, uid) # create the thumb before storing # to prevent db race conditions - thumb = Alchemy::PictureThumb.create!( - picture: variant.picture, - signature: signature, - uid: uid, - ) + @thumb = Alchemy::PictureThumb.create_or_find_by!(signature: signature) do |thumb| + thumb.picture = variant.picture + thumb.uid = uid + end begin # process the image image = variant.image @@ -32,7 +31,7 @@ def call(variant, signature, uid) rescue RuntimeError => e ErrorTracking.notification_handler.call(e) # destroy the thumb if processing or storing fails - thumb&.destroy + @thumb&.destroy end end diff --git a/spec/models/alchemy/picture_thumb/create_spec.rb b/spec/models/alchemy/picture_thumb/create_spec.rb index 8f02c43b10..92ced8e392 100644 --- a/spec/models/alchemy/picture_thumb/create_spec.rb +++ b/spec/models/alchemy/picture_thumb/create_spec.rb @@ -15,6 +15,20 @@ expect { create }.to change { variant.picture.thumbs.reload.length }.by(1) end + context "with a thumb already existing" do + let!(:thumb) do + Alchemy::PictureThumb.create!( + picture: picture, + signature: "1234", + uid: "/pictures/#{picture.id}/1234/image.png", + ) + end + + it "does not create a new thumb" do + expect { create }.to_not change { picture.thumbs.reload.length } + end + end + context "with an invalid picture" do let(:picture) { FactoryBot.build(:alchemy_picture) } From d47d2ef22169a7b69bbac4462b94fa75de4baa26 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Sun, 26 Feb 2023 12:45:23 +0100 Subject: [PATCH 3/3] Connect to writing database during thumbnail generation In a multi db setup where you have a reading and a writing database we need to make sure that we are using the writing database. This is necessary, because we are writing (caching) picture thumbnails in a GET request. Usually Rails would handle this for us on POST, PATCH/PUT and DELETE requests. ActiveStorage does the same thing to support multi database setups. Closes #2428 --- app/models/alchemy/picture/url.rb | 4 +++- spec/models/alchemy/picture/url_spec.rb | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/models/alchemy/picture/url.rb b/app/models/alchemy/picture/url.rb index c1423e5502..c819c74d1a 100644 --- a/app/models/alchemy/picture/url.rb +++ b/app/models/alchemy/picture/url.rb @@ -35,7 +35,9 @@ def uid thumb.uid else uid = PictureThumb::Uid.call(signature, variant) - PictureThumb.generator_class.call(variant, signature, uid) + ActiveRecord::Base.connected_to(role: ActiveRecord::Base.writing_role) do + PictureThumb.generator_class.call(variant, signature, uid) + end uid end end diff --git a/spec/models/alchemy/picture/url_spec.rb b/spec/models/alchemy/picture/url_spec.rb index 65f969d1f9..6a8914fe22 100644 --- a/spec/models/alchemy/picture/url_spec.rb +++ b/spec/models/alchemy/picture/url_spec.rb @@ -34,5 +34,11 @@ it "returns the url to the thumbnail" do is_expected.to match(/\/pictures\/\d+\/.+\/image\.png/) end + + it "connects to writing database" do + writing_role = ActiveRecord::Base.writing_role + expect(ActiveRecord::Base).to receive(:connected_to).with(role: writing_role) + subject + end end end