Handle RecordNotUnique error correctly (#1081)

In the find_or_create_all_with_like_by_name method,
instead of issuing a ROLLBACK statement, we wrap the tag creation in a
transaction instead.

Issuing a ROLLBACK will break nested transactions, and also does not
respect multiple databases (available since Rails 6).

Adds a spec which attempts to simulate concurrent tag creation. Adds
spec option to use truncation database strategy so the spec can see
committed records from another thread

Co-authored-by: Thong Kuah <tkuah@gitlab.com>

lib/acts_as_taggable_on/tag.rb

@@ -84,10 +84,11 @@ def self.find_or_create_all_with_like_by_name(*list)
         tries ||= 3
         comparable_tag_name = comparable_name(tag_name)
         existing_tag = existing_tags.find { |tag| comparable_name(tag.name) == comparable_tag_name }
-        existing_tag || create(name: tag_name)
+        next existing_tag if existing_tag
+
+        transaction(requires_new: true) { create(name: tag_name) }
       rescue ActiveRecord::RecordNotUnique
         if (tries -= 1).positive?
-          ActiveRecord::Base.connection.execute 'ROLLBACK'
           existing_tags = named_any(list)
           retry
         end

spec/acts_as_taggable_on/tag_spec.rb

@@ -182,6 +182,25 @@
     it 'should return an empty array if no tags are specified' do
       expect(ActsAsTaggableOn::Tag.find_or_create_all_with_like_by_name([])).to be_empty
     end
+
+    context 'another tag is created concurrently', :database_cleaner_delete, if: supports_concurrency?  do
+      it 'retries and finds tag if tag with same name created concurrently' do
+        tag_name = 'super'
+
+        expect(ActsAsTaggableOn::Tag).to receive(:create).with(name: tag_name) do
+          # Simulate concurrent tag creation
+          Thread.new do
+            ActsAsTaggableOn::Tag.new(name: tag_name).save!
+          end.join
+
+          raise ActiveRecord::RecordNotUnique
+        end
+
+        expect {
+          ActsAsTaggableOn::Tag.find_or_create_all_with_like_by_name(tag_name)
+        }.to change(ActsAsTaggableOn::Tag, :count).by(1)
+      end
+    end
   end
 
   it 'should require a name' do

spec/acts_as_taggable_on/taggable_spec.rb

@@ -640,26 +640,29 @@
 
     end
 
-    xit 'should not duplicate tags added on different threads', if: supports_concurrency?, skip: 'FIXME, Deadlocks in travis' do
-      #TODO, try with more threads and fix deadlock
-      thread_count = 4
-      barrier = Barrier.new thread_count
+    it 'should not duplicate tags' do
+      connor = TaggableModel.new(name: 'Connor', tag_list: 'There, can, be, only, one')
 
-      expect {
-        thread_count.times.map do |idx|
-          Thread.start do
-            connor = TaggableModel.first_or_create(name: 'Connor')
-            connor.tag_list = 'There, can, be, only, one'
-            barrier.wait
-            begin
-              connor.save
-            rescue ActsAsTaggableOn::DuplicateTagError
-              # second save should succeed
-              connor.save
-            end
-          end
-        end.map(&:join)
-      }.to change(ActsAsTaggableOn::Tag, :count).by(5)
+      allow(ActsAsTaggableOn::Tag).to receive(:create).and_call_original
+      expect(ActsAsTaggableOn::Tag).to receive(:create).with(name: 'can') do
+        # Simulate concurrent tag creation
+        ActsAsTaggableOn::Tag.new(name: 'can').save!
+
+        raise ActiveRecord::RecordNotUnique
+      end
+
+      expect(ActsAsTaggableOn::Tag).to receive(:create).with(name: 'be') do
+        # Simulate concurrent tag creation
+        ActsAsTaggableOn::Tag.new(name: 'be').save!
+
+        raise ActiveRecord::RecordNotUnique
+      end
+
+      expect { connor.save! }.to change(ActsAsTaggableOn::Tag, :count).by(5)
+
+      %w[There can only be one].each do |tag|
+        expect(TaggableModel.tagged_with(tag).count).to eq(1)
+      end
     end
   end
 

spec/support/database_cleaner.rb

@@ -6,6 +6,10 @@
     DatabaseCleaner.clean
   end
 
+  config.before(:each, :database_cleaner_delete) do
+    DatabaseCleaner.strategy = :truncation
+  end
+
   config.after(:suite) do
     DatabaseCleaner.clean
   end