This project is archived and is in readonly mode.
ActionController::Caching::Sweeper controller instance is not thread safe
Since Sweeper is a singleton(derived from
ActiveRecord::Observer or
ActiveModel::Observer edge) it seems that multiple
threads could modify the single controller instance set and cleared
in the around_filter.
class Sweeper < ActiveRecord::Observer #:nodoc:
attr_accessor :controller
def before(controller)
self.controller = controller
callback(:before) if controller.perform_caching
end
def after(controller)
callback(:after) if controller.perform_caching
# Clean up, so that the controller can be collected after this request
self.controller = nil
end
Would using Thread.current be preferable?
-
sweeper_thread_patch.diff
1.48 KB
-
-
Michael Rykov
This often yields to the following errors when two requests overlap on the sweeper
NoMethodError (undefined method `controller_name' for nil:NilClass): ...actionpack-2.3.5/lib/action_controller/caching/sweeper.rb:32:in `callback' ...actionpack-2.3.5/lib/action_controller/caching/sweeper.rb:14:in `after' ...actionpack-2.3.5/lib/action_controller/filters.rb:208:in `around_proc'Workaround:
class ActionController::Caching::Sweeper def controller Thread.current[:"#{self}_controller"] end def controller=(c) Thread.current[:"#{self}_controller"] = c end end -
Ryan Bigg- State changed from new to open
- Importance changed from to
-
Greg Hazel+1
Ran in to this (several times a second...) when trying to use Rails with config.threadsafe!
-
Michael KoziarskiI'd take this as fix for 2-3-stable and possibly 3-0-stable. but for master we should probably do this right and have a sweeper instance per controller rather than a weird 'instance variable backed by a thread local'. There's a reason why we have a controller per request rather than request, session, etc all being in Thread.current.
Care to take a stab at that larger refactoring?
-
Matt DRunning into this issue regularly in production. +1