From 5d7cab67b65807ecbb56e5552871ffed5da34726 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Janko=20Marohni=C4=87?= <janko.marohnic@gmail.com>
Date: Sun, 13 Mar 2016 14:26:45 +0700
Subject: [PATCH] Make swap impenetrable by concurrency

So, currently, these is still a teeny-tiny chance that on subsequent
attachment updates an older update overwrites a newer, if another UPDATE
happens between reloading and updating. I've tried preventing that with
a database transaction, but I apparently back then didn't understand
transactions good because I thought they act like mutexes.

The right way to make "check and update" atomic is simply making an
UPDATE...WHERE query. Then afterwards we reload the record so that it
has the actual attachment value, and then we just check whether the
attachment value is our stored file. If it's not, we return nil, so that
`Attacher#promote` knows that the swap failed and knows to delete the
unused stored file.

Hopefully this should now ensure that there are no concurrency issues.
---
 CHANGELOG.md                       |  2 ++
 lib/shrine.rb                      |  2 +-
 lib/shrine/plugins/activerecord.rb | 16 +++++-----------
 lib/shrine/plugins/sequel.rb       | 16 +++++-----------
 test/attacher_test.rb              |  2 +-
 5 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4b91a6e56..d6eff4cc9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## HEAD
 
+* Remove a tiny possibility of a race condition with backgrounding on subsequent updates (janko-m)
+
 * Add `:delegate` option to migration_helpers for opting out of defining methods on the model (janko-m)
 
 * Make logging plugin log number of both input and output files for processing (janko-m)
diff --git a/lib/shrine.rb b/lib/shrine.rb
index 5e258861a..eb27d9b9d 100644
--- a/lib/shrine.rb
+++ b/lib/shrine.rb
@@ -590,7 +590,7 @@ def promote?(uploaded_file)
         # Calls #update, overriden in ORM plugins.
         def swap(uploaded_file)
           update(uploaded_file)
-          uploaded_file
+          uploaded_file if get == uploaded_file
         end
 
         # Sets and saves the uploaded file.
diff --git a/lib/shrine/plugins/activerecord.rb b/lib/shrine/plugins/activerecord.rb
index 4a0f9149e..6269e25bc 100644
--- a/lib/shrine/plugins/activerecord.rb
+++ b/lib/shrine/plugins/activerecord.rb
@@ -75,18 +75,12 @@ module AttacherMethods
 
         # Updates the current attachment with the new one, unless the current
         # attachment has changed.
-        def swap(uploaded_file)
-          record.class.transaction do
-            break if record.send("#{name}_data") != record.reload.send("#{name}_data")
-            super
-          end
-        rescue ::ActiveRecord::RecordNotFound
-        end
-
-        # We save the record after updating, raising any validation errors.
         def update(uploaded_file)
-          super
-          record.save!
+          record.class.where(record.class.primary_key => record.id)
+            .where(:"#{name}_data" => record.send(:"#{name}_data"))
+            .update_all(:"#{name}_data" => uploaded_file.to_json)
+          record.reload
+        rescue ::ActiveRecord::RecordNotFound
         end
       end
     end
diff --git a/lib/shrine/plugins/sequel.rb b/lib/shrine/plugins/sequel.rb
index e2b531609..ed93f6817 100644
--- a/lib/shrine/plugins/sequel.rb
+++ b/lib/shrine/plugins/sequel.rb
@@ -73,18 +73,12 @@ module AttacherMethods
 
         # Updates the current attachment with the new one, unless the current
         # attachment has changed.
-        def swap(uploaded_file)
-          record.db.transaction do
-            break if record.send("#{name}_data") != record.reload.send("#{name}_data")
-            super
-          end
-        rescue ::Sequel::Error
-        end
-
-        # We save the record after updating, raising any validation errors.
         def update(uploaded_file)
-          super
-          record.save(raise_on_failure: true)
+          record.this
+            .where(:"#{name}_data" => record.send(:"#{name}_data"))
+            .update(:"#{name}_data" => uploaded_file.to_json)
+          record.reload
+        rescue ::Sequel::Error
         end
 
         # Support for Postgres JSON columns.
diff --git a/test/attacher_test.rb b/test/attacher_test.rb
index ef77b9b9e..2f7b917f6 100644
--- a/test/attacher_test.rb
+++ b/test/attacher_test.rb
@@ -139,7 +139,7 @@
       io = fakeio
       context = {name: @attacher.name, record: @attacher.record, phase: :store}
       @attacher.assign(io)
-      @attacher.store.expects(:upload).with(@attacher.get, context).returns(true)
+      @attacher.store.expects(:upload).with(@attacher.get, context).returns(@attacher.get)
       @attacher._promote
     end