Rails - Jun 2008 - after-destroy + counter_cache concurrency issues; seeking feedback on potential solution

We''ve run across the following concurrency issue with after-destroy:

We have an Post model that has_many Recommendations and uses a
counter_cache:

class Post < ActiveRecord::Base
  has_many :recommendations, :counter_cache => true
end

And, in a controller, we have a delete_recommendation action:

def delete_recommendation
  r = Recommendation.find(params[:id])
  r.destroy
end

Most of the time, the recommendation counter is updated properly. But
if many delete_recommendation requests are processed at once for the
same recommendation object, then our counts can get out of sync. The
problem is that, in many of the simultaneous requests, the
Recommendation.find(params[:id]) line successfully retrieves the
object, and is therefore able to call destroy on it. Obviously only
one of the resultant "DELETE FROM" SQL calls will actually remove any
rows from the database, but the after_destroy callback will be called
for every request regardless, which means the count will be improperly
decremented multiple times.

There are many ways to work around this problem, but this particular
pattern is common, and it seems like the Rails counter_cache
implementation ought handle this case. It occurred to us that the
problem would not occur if the after_destroy callback was only called
when an object was actually deleted -- i.e., the callback would only
be called if the number of rows affected by the destroy call is
greater than 0. Does this behavior sound reasonable? Should we propose
making this change to ActiveRecord? Are there any scenarios where you
would want the after_destroy callback to be called even if destroy did
not delete anything from the database?

Michael
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

On Jun 16, 8:30 pm, Michael
<lov...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:
> pattern is common, and it seems like the Rails counter_cache
> implementation ought handle this case. It occurred to us that the
> problem would not occur if the after_destroy callback was only called
> when an object was actually deleted -- i.e., the callback would only
> be called if the number of rows affected by the destroy call is
> greater than 0. Does this behavior sound reasonable? Should we propose
> making this change to ActiveRecord? Are there any scenarios where you
> would want the after_destroy callback to be called even if destroy did
> not delete anything from the database?
I had a quick peek at the source and it seems there that the counter
is decremented from the before_destroy callback, so messing with the
after_destroy isn''t going to help. You might want to ask this on
rubyonrails-core.


Fred

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

Frederick, thanks for the correction. I had misread the relevant Rails
source code.

So I would like to change my proposed solution to:

* Move the counter decrement from the before_destroy callback to the
after_destroy callback
* Only call the after_destroy callback if the call to destroy actually
deleted rows from the database

If I don''t receive a response here, I''ll re-post this question
to
rubyonrails-core.

Michael

On Jun 16, 3:10 pm, Frederick Cheung
<frederick.che...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:
> On Jun 16, 8:30 pm, Michael
<lov...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> > pattern is common, and it seems like the Rails counter_cache
> > implementation ought handle this case. It occurred to us that the
> > problem would not occur if the after_destroy callback was only called
> > when an object was actually deleted -- i.e., the callback would only
> > be called if the number of rows affected by the destroy call is
> > greater than 0. Does this behavior sound reasonable? Should we propose
> > making this change to ActiveRecord? Are there any scenarios where you
> > would want the after_destroy callback to be called even if destroy did
> > not delete anything from the database?
>
> I had a quick peek at the source and it seems there that the counter
> is decremented from the before_destroy callback, so messing with the
> after_destroy isn''t going to help. You might want to ask this on
> rubyonrails-core.
>
> Fred
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

Quoting Michael
<lovitt-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 
> Frederick, thanks for the correction. I had misread the relevant Rails
> source code.
> 
> So I would like to change my proposed solution to:
> 
> * Move the counter decrement from the before_destroy callback to the
> after_destroy callback
> * Only call the after_destroy callback if the call to destroy actually
> deleted rows from the database
> 
In the after_destroy callback, the object is frozen, including associations.
So changing the parent isn''t allowed.

Jeffrey

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk-unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

I think now you can patch your rails:
http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/1134-counter_cache-destroy-concurrency-issues
-- 
Posted via http://www.ruby-forum.com/.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---