find_or_create_by: handle race condition by finding again #45720
Conversation
I have seen tests in AR using |
|
@simi thanks, but my problem is not so much the construct to use to synchronize threads (a simple I guess we could write a test that inline the method to make it easier, but then it's not longer really testing the method, and it more or less a copy of the |
|
Usually I do subclass in test and hijack original method definition, but indeed that is not testing the original method. Other idea would be to somehow intercept the |
|
Yeah, I guess Far from perfect, but good enough. |
casperisfine
force-pushed
the
find-or-create-or-find-by
branch
3 times, most recently
from
80d1566
to
3fdc867
Compare
|
Ok, I added a mock based test that hopefully shouldn't be too fragile. |
activerecord/lib/active_record/encryption/extended_deterministic_queries.rb
Outdated
Show resolved
Hide resolved
| @@ -161,31 +161,27 @@ 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 was a problem hiding this comment.
I think this warning is probably worth keeping... if you don't have a uniqueness constraint defined, it will mostly seem to work but have the described race.
There was a problem hiding this comment.
Hum, I don't think it should be kept as is, but yes, rather than to remove it I should rewod it to say that if you don't have a unique constaint, you'll end up with two records.
There was a problem hiding this comment.
Ok, I reworded the documentation, let me know what you think.
casperisfine
force-pushed
the
find-or-create-or-find-by
branch
3 times, most recently
from
7588a2f
to
8d27891
Compare
|
Also related: #44885 |
casperisfine
force-pushed
the
find-or-create-or-find-by
branch
from
8d27891
to
41e1f74
Compare
|
Credited @alexcameron89 as co-author because ultimately our PRs are pretty much identical and there was no reason #35633 went stale. |
Using `create_or_find_by` in codepaths where most of the time the record already exist is wasteful on several accounts. `create_or_find_by` should be the method to use when most of the time the record doesn't already exist, not a race condition safe version of `find_or_create_by`. To make `find_or_create_by` race-condition free, we can search the record again if the creation failed because of an unicity constraint. Co-Authored-By: Alex Kitchens <alexcameron98@gmail.com>
casperisfine
force-pushed
the
find-or-create-or-find-by
branch
from
41e1f74
to
023a3eb
Compare
|
I like this. My one reservation is that it could change user's expectations around callbacks or especially the block passed to That reservation aside (which is really and edge case, maybe we should document this possibility), strong 👍 |
|
This pull request can introduce subtransactions in PostgreSQL, which can cause significant performance issues. Please see #51052. |
Using
create_or_find_byin codepaths where most of the time the record already exist is wasteful on several accounts.create_or_find_byshould be the method to use when most of the time the record doesn't already exist, not a race condition safe version offind_or_create_by.To make
find_or_create_byrace-condition free, we can search the record again if the creation failed because of an unicity constraint.This PR was sparked by a recent comment from @jhawthorn.
Apparently this change was considered in #35543 by @matthewd, and #35633 was opened, but it went stale.
Ref: #35543
Ref: #35573
Close: #44885
NB: I have no idea how we could unit test the race-condition. Ideas welcome.