need advice about race condition

rubyonrails-talk
1

There's a field in my User model that sometimes needs bulk updates:

   User.update_all("cluster = #{new_cluster}", "cluster = #{current_cluster}")

Now, suppose user with id 37 is affected by that particular update_all and updates his profile:

   1. user 37 is read    2. update_all happens and changes 37's cluster asynchronously    3. user 37 is saved

now user 37 belongs to the wrong cluster. Unlikely but possible. Is there a robust way to write that?

-- fxn

2

Xavier Noria wrote:

There's a field in my User model that sometimes needs bulk updates:

   User.update_all("cluster = #{new_cluster}", "cluster = # {current_cluster}")

Now, suppose user with id 37 is affected by that particular
update_all and updates his profile:

   1. user 37 is read    2. update_all happens and changes 37's cluster asynchronously    3. user 37 is saved

now user 37 belongs to the wrong cluster. Unlikely but possible. Is
there a robust way to write that?

You might want to read:

zsombor

3

Does user 37 have the ability to change his own cluster? If not, then you’ll only update the fields that your form submits… not everything. (provided you use update_attributes to save the results)

If he can update his cluster, then the only thing that comes to mind is to look at the modified date/time of the record you have out and compare it to what’s in the database.

Add a field to your table called updated_at

then in your User model, do a callback

before_update :check_if_stale

protected def check_if_stale user_updated_at = User.find( self.id).updated_at if user_updated_at == self.updated_at #record is ok to update else # your copy is stale… better do some corrective stuff… could return false which would prevent the record from being saved.

end end

Not sure if that’s the best approach… someone else could have a cleaner way of doing that.

4

Oh yes, I had forgotten lock_version. Thank you!

-- fxn

5

Yeah, I don't want to force all the system to use update_attributes though, sounds brittle. I prefer a solution that still lets people use AR models normally (except for non-bang-saves, who can raise now an exception as save! does).

-- fxn

6

Too quick, since that field is changed by an update_all lock_version is untouched. I could do a select+assign in a before_save but there's still room for a race condition there, hmmmmm.

-- fxn

7

@Xavier:

Did you try my solution that used the updated_at column in the before_update?

8

That solution has another race condition, the fatal sequence within before_update is:

   1. updated_at is read and coincides, fine    2. update_all happens in a separate process    3. save --> wrong

which is the same sequence as in the more straightforward

   1. cluster is read and coincides, fine    2. update_all happens in a separate process    3. save --> wrong

Perhaps that approach plus a mandatory transaction in all user updates will be the only robust solution.

-- fxn

9

I was kind of assuming callbacks did not run in a transaction, but they actually do according to

   http://api.rubyonrails.com/classes/ActiveRecord/Transactions/ClassMethods.html

so I think I am pretty close to getting this right, great!

-- fxn

10

Your original update_all is sufficient if you include lock_version=lock_version+1. Then dirty readers will get a StaleObjectError if they save after the cluster change.

jeremy