Merge pull request #45720 from Shopify/find-or-create-or-find-by

find_or_create_by: handle race condition by finding again

activerecord/CHANGELOG.md

@@ -1,3 +1,21 @@
+*   `find_or_create_by` now try to find a second time if it hits a unicity constraint.
+
+    `find_or_create_by` always has been inherently racy, either creating multiple
+    duplicate records or failing with `ActiveRecord::RecordNotUnique` depending on
+    whether a proper unicity constraint was set.
+
+    `create_or_find_by` was introduced for this use case, however it's quite wasteful
+    when the record is expected to exist most of the time, as INSERT require to send
+    more data than SELECT and require more work from the database. Also on some
+    databases it can actually consume a primary key increment which is undesirable.
+
+    So for case where most of the time the record is expected to exist, `find_or_create_by`
+    can be made race-condition free by re-trying the `find` if the `create` failed
+    with `ActiveRecord::RecordNotUnique`. This assumes that the table has the proper
+    unicity constraints, if not, `find_or_create_by` will still lead to duplicated records.
+
+    *Jean Boussier*, *Alex Kitchens*
+
 *   Introduce a simpler constructor API for ActiveRecord database adapters.
 
     Previously the adapter had to know how to build a new raw connection to

activerecord/lib/active_record/relation.rb

@@ -161,21 +161,25 @@ def first_or_initialize(attributes = nil, &block) # :nodoc:
     # failed due to validation errors it won't be persisted, you get what
     # #create returns in such situation.
     #
-    # Please note <b>this method is not atomic</b>, it runs first a SELECT, and if
-    # there are no results an INSERT is attempted. If there are other threads
-    # or processes there is a race condition between both calls and it could
-    # be the case that you end up with two similar records.
+    # If creation failed because of a unique constraint, this method will
+    # assume it encountered a race condition and will try finding the record
+    # once more If somehow the second find still find no record because a
+    # concurrent DELETE happened, it will then raise an
+    # <tt>ActiveRecord::RecordNotFound</tt> exception.
     #
-    # If this might be a problem for your application, please see #create_or_find_by.
+    # Please note <b>this method is not atomic</b>, it runs first a SELECT,
+    # and if there are no results an INSERT is attempted. So if the table
+    # doesn't have a relevant unique constraint it could be the case that
+    # you end up with two or more similar records.
     def find_or_create_by(attributes, &block)
-      find_by(attributes) || create(attributes, &block)
+      find_by(attributes) || create_or_find_by(attributes, &block)
     end
 
     # Like #find_or_create_by, but calls
     # {create!}[rdoc-ref:Persistence::ClassMethods#create!] so an exception
     # is raised if the created record is invalid.
     def find_or_create_by!(attributes, &block)
-      find_by(attributes) || create!(attributes, &block)
+      find_by(attributes) || create_or_find_by!(attributes, &block)
     end
 
     # Attempts to create a record with the given attributes in a table that has a unique database constraint
@@ -183,9 +187,8 @@ def find_or_create_by!(attributes, &block)
     # unique constraints, the exception such an insertion would normally raise is caught,
     # and the existing record with those attributes is found using #find_by!.
     #
-    # This is similar to #find_or_create_by, but avoids the problem of stale reads between the SELECT
-    # and the INSERT, as that method needs to first query the table, then attempt to insert a row
-    # if none is found.
+    # This is similar to #find_or_create_by, but tries to create the record first. As such it is
+    # better suited for cases where the record is most likely not to exist yet.
     #
     # There are several drawbacks to #create_or_find_by, though:
     #

activerecord/test/cases/relations_test.rb

@@ -1483,6 +1483,27 @@ def test_find_or_create_by
     assert_equal bird, Bird.find_or_create_by(name: "bob")
   end
 
+  def test_find_or_create_by_race_condition
+    assert_nil Subscriber.find_by(nick: "bob")
+
+    bob = Subscriber.create!(nick: "bob")
+    relation = Subscriber.all
+
+    results = [nil, bob]
+    find_by_mock = -> (*) do
+      assert_not_predicate results, :empty?
+      results.shift
+    end
+
+    relation.stub(:find_by, find_by_mock) do
+      relation.stub(:find_by!, find_by_mock) do # create_or_find_by always call find_by! on retry
+        assert_equal bob, relation.find_or_create_by(nick: "bob")
+      end
+    end
+
+    assert_predicate results, :empty?
+  end
+
   def test_find_or_create_by_with_create_with
     assert_nil Bird.find_by(name: "bob")