fix for race condition when multiple processes try to add the same tag

acts-as-taggable-on.gemspec

@@ -33,5 +33,5 @@ Gem::Specification.new do |gem|
   gem.add_development_dependency 'rspec-rails', '2.13.0' # 2.13.1 is broken
   gem.add_development_dependency 'rspec', '~> 2.6'
   gem.add_development_dependency 'ammeter'
-
+  gem.add_development_dependency 'barrier'
 end

lib/acts-as-taggable-on.rb

@@ -6,6 +6,9 @@
 require "digest/sha1"
 
 module ActsAsTaggableOn
+  class DuplicateTagError < StandardError
+  end
+
   def self.setup
     @configuration ||= Configuration.new
     yield @configuration if block_given?

lib/acts_as_taggable_on/acts_as_taggable_on/core.rb

@@ -399,6 +399,10 @@ def save_tags
       end
 
       true
+
+    rescue StandardError => e
+      self.errors.add(:base, "Error saving tags: #{e.message}")
+      false
     end
   end
 end

lib/acts_as_taggable_on/tag.rb

@@ -73,11 +73,34 @@ def self.find_or_create_all_with_like_by_name(*list)
 
       existing_tags = Tag.named_any(list)
 
-      list.map do |tag_name|
+      result = []
+      duplicates = []
+      list.each do |tag_name|
         comparable_tag_name = comparable_name(tag_name)
         existing_tag = existing_tags.detect { |tag| comparable_name(tag.name) == comparable_tag_name }
 
-        existing_tag || Tag.create(:name => tag_name)
+        if existing_tag
+          result << existing_tag
+        else
+          begin
+            result << Tag.create(:name => tag_name)
+          rescue ActiveRecord::RecordNotUnique
+            # Postgres aborts the current transaction with
+            # PG::InFailedSqlTransaction: ERROR:  current transaction is aborted, commands ignored until end of transaction block
+            # so we have to rollback this transaction
+            if ActsAsTaggableOn::Tag.aborts_on_duplicate?
+              raise DuplicateTagError.new("'#{tag_name}' has already been taken")
+            end
+            # MySQL we can request the duplicates again later.
+            duplicates << tag_name
+          end
+        end
+      end
+
+      if duplicates.empty?
+        result
+      else
+        result.concat(Tag.named_any(duplicates))
       end
     end
 
@@ -114,7 +137,7 @@ def unicode_downcase(string)
         if ActiveSupport::Multibyte::Unicode.respond_to?(:downcase)
           ActiveSupport::Multibyte::Unicode.downcase(string)
         else
-          ActiveSupport::Multibyte::Chars.new(string).downcase.to_s  
+          ActiveSupport::Multibyte::Chars.new(string).downcase.to_s
         end
       end
 

lib/acts_as_taggable_on/utils.rb

@@ -22,6 +22,14 @@ def using_case_insensitive_collation?
       using_mysql? && ::ActiveRecord::Base.connection.collation =~ /_ci\Z/
     end
 
+    def aborts_on_duplicate?
+      using_postgresql?
+    end
+
+    def supports_concurrency?
+      !using_sqlite?
+    end
+
     def sha_prefix(string)
       Digest::SHA1.hexdigest("#{string}#{rand}")[0..6]
     end

spec/acts_as_taggable_on/tag_spec.rb

@@ -80,6 +80,14 @@
       it "should find by name case insensitive" do
         ActsAsTaggableOn::Tag.find_or_create_with_like_by_name("ПРИВЕТ").should == @tag
       end
+
+      if ActsAsTaggableOn::Tag.using_case_insensitive_collation?
+        it "should find by name accent insensitive" do
+          @tag.name = "inupiat"
+          @tag.save
+          ActsAsTaggableOn::Tag.find_or_create_with_like_by_name("Iñupiat").should == @tag
+        end
+      end
     end
   end
 

spec/acts_as_taggable_on/taggable_spec.rb

@@ -246,7 +246,7 @@
   unless ActsAsTaggableOn::Tag.using_sqlite?
     it "should not care about case for unicode names" do
       ActsAsTaggableOn.strict_case_match = false
-  
+
       anya = TaggableModel.create(:name => "Anya", :tag_list => "ПРИВЕТ")
       igor = TaggableModel.create(:name => "Igor", :tag_list => "привет")
       katia = TaggableModel.create(:name => "Katia", :tag_list => "ПРИВЕТ")
@@ -269,7 +269,7 @@
     TaggableModel.tagged_with("中国的").to_a.size.should == 1
     TaggableModel.tagged_with("العربية").to_a.size.should == 1
     TaggableModel.tagged_with("✏").to_a.size.should == 1
-  end 
+  end
 
   it "should be able to get tag counts on model as a whole" do
     bob = TaggableModel.create(:name => "Bob", :tag_list => "ruby, rails, css")
@@ -482,13 +482,43 @@
     TaggableModel.tagged_with([]).should == []
   end
 
-  it "should not create duplicate taggings" do
-    bob = TaggableModel.create(:name => "Bob")
-    lambda {
-      bob.tag_list << "happier"
-      bob.tag_list << "happier"
-      bob.save
-    }.should change(ActsAsTaggableOn::Tagging, :count).by(1)
+  describe "Duplicates" do
+    it "should not create duplicate taggings" do
+      bob = TaggableModel.create(:name => "Bob")
+      lambda {
+        bob.tag_list << "happier"
+        bob.tag_list << "happier"
+        bob.save
+      }.should change(ActsAsTaggableOn::Tagging, :count).by(1)
+    end
+
+    if ActsAsTaggableOn::Tag.supports_concurrency?
+      it "should not duplicate tags added on different threads" do
+        thread_count = 4
+        barrier = Barrier.new thread_count
+
+        results = []
+        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
+              connor.save
+              results[idx] = connor.errors.full_messages
+            end
+          end.map(&:join)
+        }.to change(ActsAsTaggableOn::Tag, :count).by(5)
+
+        errors = results.flatten
+        if ActsAsTaggableOn::Tag.aborts_on_duplicate?
+          # only one thread succeeds because the transaction is aborted in this case
+          errors.should eq Array.new(thread_count - 1, "Error saving tags: 'There' has already been taken")
+        else
+          errors.should be_empty
+        end
+      end
+    end
   end
 
   describe "Associations" do

spec/spec_helper.rb

@@ -5,6 +5,7 @@
 require File.expand_path('../../lib/acts-as-taggable-on', __FILE__)
 I18n.enforce_available_locales = true
 require 'ammeter/init'
+require 'barrier'
 
 unless [].respond_to?(:freq)
   class Array