I don't think it's reasonable to force pessimistic locks on every single destroy call, my point is more that in your case it's a work around.
-- Cheers, Koz On Wednesday, 17 October 2012 at 7:06 AM, Brian Durand wrote: > I think adding a pessimistic lock to the destroy method will work. I opened > this pull request that locks the record in the database before destroying it. > If the record no longer exists, the callbacks are not called. > > https://github.com/rails/rails/pull/7965 > > This won't work on databases that don't support row locking, but you're using > such a database you'd likely have other issues in a high concurrency > situation like is needed to produce this issue. > > > On Saturday, October 13, 2012 10:47:15 AM UTC-7, Brian Durand wrote: > > I've put a stand alone script here that reproduces the issue: > > > > https://gist.github.com/3885509 > > > > > > On Friday, October 12, 2012 1:36:22 PM UTC-7, richard schneeman wrote: > > > Can you write a public rails app that reproduces this issue? This > > > behavior would be undesired and therefore a bug. If we can reproduce and > > > attach that to an issue it could help the discussion. > > > > > > -- > > > Richard Schneeman > > > http://heroku.com > > > > > > @schneems (http://twitter.com/schneems) > > > > > > > > > > > > > > > On Friday, October 12, 2012 at 7:53 AM, Brian Durand wrote: > > > > > > > I've been looking into consistency problem with association counter > > > > caches where the counter cache value in the database is not consistent > > > > with the actual number of records in the association. What I've found > > > > is that it is from a concurrency issue where two process try to destroy > > > > the same record at the same time. Here is the pseudo SQL that is sent > > > > to the database when two process are deleting at the same time: > > > > > > > > process_1 -> SELECT * FROM table WHERE id = 1 > > > > process_2 -> SELECT * FROM table WHERE id = 1 > > > > process_1 -> BEGIN > > > > process_2 -> BEGIN > > > > process_1 -> UPDATE parent_table SET counter_cache = > > > > COALESCE(counter_cache, 0) - 1 WHERE id = 1 > > > > process_1 -> DELETE FROM table WHERE id = 1 > > > > process_1 -> COMMIT > > > > process_2 -> UPDATE parent_table SET counter_cache = > > > > COALESCE(counter_cache, 0) - 1 WHERE id = 1 > > > > process_2 -> DELETE FROM table WHERE id = 1 > > > > process_2 -> COMMIT > > > > > > > > What happens is process_1 updates the counter cache and deletes the > > > > record. Process_2 simply updates the counter cache because the record > > > > is already deleted by the time it tries to delete it. > > > > > > > > This is a pretty complicated issue and it touches more than just this > > > > one test case. The problem being that all the before and after destroy > > > > callbacks will be called regardless of if the record is actually > > > > destroyed. In the particular case of the counter caches, I think it > > > > could be fixed by moving the callback from a before_destroy to an > > > > after_destroy and adding a check in ActiveRecord to only call after > > > > destroy callbacks if a row was actually removed from the table. > > > > > > > > In general I think it would be correct to make this general behavior so > > > > that after_destroy callbacks are not called if no record was deleted. > > > > However, that could affect quite a few things inside application code > > > > which could potentially leave objects in an inconsistent state because > > > > an expected callback was not called. I think the pending upgrade to > > > > Rails 4.0 might be a good time to introduce such behavior since it's a > > > > major upgrade and as such people should not be expecting applications > > > > to work 100% without some alterations. This does not touch on the issue > > > > of before_destroy callbacks which would not be able to check the status > > > > of the delete operation. This could be handled with documentation > > > > stating that this is a known issue. > > > > > > > > Another solution that would have less effect on current applications > > > > (but also leave them more vulnerable to being in an inconsistent state) > > > > would be to provide some sort of flag within the record that > > > > after_destroy callbacks could check if they are persisting data or > > > > interacting with external systems. Something like "row_deleted?" so > > > > that callbacks could be defined as: > > > > > > > > after_destroy :my_callback, :if => :row_deleted? > > > > > > > > Thoughts? > > > > > > > > -- > > > > You received this message because you are subscribed to the Google > > > > Groups "Ruby on Rails: Core" group. > > > > To view this discussion on the web visit > > > > https://groups.google.com/d/msg/rubyonrails-core/-/KnPOlQzxj2cJ. > > > > To post to this group, send email to [email protected]. > > > > To unsubscribe from this group, send email to > > > > [email protected]. > > > > For more options, visit this group at > > > > http://groups.google.com/group/rubyonrails-core?hl=en. > > > > -- > You received this message because you are subscribed to the Google Groups > "Ruby on Rails: Core" group. > To view this discussion on the web visit > https://groups.google.com/d/msg/rubyonrails-core/-/MyWn00d6O-MJ. > To post to this group, send email to [email protected] > (mailto:[email protected]). > To unsubscribe from this group, send email to > [email protected] > (mailto:[email protected]). > For more options, visit this group at > http://groups.google.com/group/rubyonrails-core?hl=en. -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.
