#3502 Reloading a new record results in an inconsistent state - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#3502 open

Reloading a new record results in an inconsistent state

Reported by Arya Asemanfar | November 22nd, 2009 @ 04:13 AM | in 2.3.10

If there is a race to create a new record for the same ID, and you try to reload, the resulting record is in an inconsistent state.

For example, if two processes do a find_or_initialize_by_id(5) and both come back with new records, then the first process saves, then the second tries and throws a ActiveRecord::RecordNotUnique. If you rescue that and do record.reload, it will load the record the first process saved, but without clearing the new_record flag. So if you try to save that record in the process that reloaded, it does an insert and throws another ActiveRecord::RecordNotUnique. The behavior is inconsistent, it should either not allow you to call reload on a new_record, or it should clear the new_record flag if the reload succeeded. The documentation for new_record? says that it returns false if a record for the object doesn't exist yet, which maybe indicates the fix should be to clear the new_record flag.

Comments and changes to this ticket

Matt Jones November 22nd, 2009 @ 07:48 AM
- Tag cleared.
Why on earth are you trying to pick an ID for the record before it's saved? The whole situation has "race condition disaster" written all over it.
Jeremy Kemper November 22nd, 2009 @ 07:50 AM
- Tag set to “activerecord, reloading”
- Assigned user set to “Jeremy Kemper”
- State changed from “new” to “open”
- Milestone set to “2.3.6”
Yes, indeed. Patch welcome for next 2.3 release!
Jeremy Kemper November 22nd, 2009 @ 07:51 AM
Matt, the scenario does have code smell, but it's broken nonetheless :)
You flagged this item as spam.
Arya Asemanfar November 22nd, 2009 @ 07:52 AM
- Assigned user cleared.
@matt Facebook application, we use their facebook user id as the id column. When you interact with a friend on the app, their record is created by their ID.
You flagged this item as spam.
Arya Asemanfar November 22nd, 2009 @ 07:54 AM
I should clarify that the ID column is not an auto-increment column.
You flagged this item as spam.
Arya Asemanfar January 18th, 2010 @ 01:29 AM
- Tag changed from “activerecord, reloading” to “activerecord, patch, reloading”
- Assigned user set to “Jeremy Kemper”
Here is a patch and test for this ticket.

My earlier comment cleared the assigned user because I had loaded the form before it was assigned, so I'm assigning it back to Jeremy. Sorry about that.
- 0001-fixed-reloading-new_record-resulting-in-an-inconsist.patch 1.7 KB
Rizwan Reza May 16th, 2010 @ 02:41 AM
- Tag changed from “activerecord, patch, reloading” to “activerecord, bugmash, patch, reloading”
[bulk edit]
You flagged this item as spam.
Anil Wadghule May 16th, 2010 @ 04:04 PM
-1

Without the fix, test doesn't fail on Rails master and Rails 2-3 stable.
You flagged this item as spam.
Bastes May 16th, 2010 @ 04:05 PM
Looks fixed already, patch does nothing.
You flagged this item as spam.
Arya Asemanfar May 16th, 2010 @ 07:14 PM
Patch doesn't apply anymore because the relevant code was moved from base.rb to persistence.rb, but the problem still exists. I just tried the test on master and it's still broken. Looking through the code also confirms that new_record is not set to false after successful reload, and save checks "new_record?" and then chooses "create" which does an insert.
You flagged this item as spam.
Colin Casey May 16th, 2010 @ 10:19 PM
+1 test does fail on master.

Rewrote Arya's patch so it applies to master. I've attached a patch and I've attached a test.
- fixed-reloading-new_record-resulting-in-an-inconsist.patch 1.9 KB
You flagged this item as spam.
Colin Casey May 17th, 2010 @ 12:03 AM
- Tag changed from “activerecord, bugmash, patch, reloading” to “activerecord, bugmash, bugmash-review, patch, reloading”
Jeremy Kemper May 23rd, 2010 @ 05:54 PM
- Milestone changed from “2.3.6” to “2.3.7”
[bulk edit]
Jeremy Kemper May 24th, 2010 @ 09:40 AM
- Milestone changed from “2.3.7” to “2.3.8”
[bulk edit]
Jeremy Kemper May 25th, 2010 @ 11:45 PM
- Milestone changed from “2.3.8” to “2.3.9”
[bulk edit]
Jeremy Kemper August 30th, 2010 @ 02:28 AM
- Milestone changed from “2.3.9” to “2.3.10”
- Importance changed from “” to “”
[bulk edit]
David Trasbo September 23rd, 2010 @ 10:56 AM
IMHO, the correct way to fix this would be to not allow reloading of new records at all. After all, the whole idea of reloading is to re-fetch the record from the database and bring it back in sync with the reloaded object. Since a new record is not yet in the database, reloading does not make sense conceptually.
You flagged this item as spam.
Andrea Campi October 8th, 2010 @ 03:43 PM
-1 I agree with David, I don't see the use of reloading a new record.
You flagged this item as spam.
Arya Asemanfar October 9th, 2010 @ 10:20 PM
I don't disagree. In fact that solution was stated in my original ticket. The reason I patched it the way I did was because my particular use case needed me to fetch the record I thought was new, but wasn't, from the database. So if I didn't do it that way, I'd have to do something like record.replace(Model.find(record.id)).

Whether or not use you a non-auto-increment PK is another argument, but that case is clearly supported. So in conclusion, it's still inconsistent, and either solution will resolve the inconsistency.
You flagged this item as spam.
Andrea Campi October 11th, 2010 @ 07:34 AM
As far as I'm concerned, I'd rather see #reload raise if passed a new_record?
Ryan Bigg October 11th, 2010 @ 12:09 PM
- Tag cleared.
Automatic cleanup of spam.
Ryan Bigg October 19th, 2010 @ 08:26 AM
Automatic cleanup of spam.