#3457 ActionController::Caching::Sweeper controller instance is not thread safe - 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.

#3457 open

ActionController::Caching::Sweeper controller instance is not thread safe

Reported by blythe | November 4th, 2009 @ 09:11 PM

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.5 KB

Comments and changes to this ticket

You flagged this item as spam.
Timothy Jones February 4th, 2010 @ 05:12 AM
+1

Michael Rykov April 7th, 2010 @ 09:07 PM

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 December 15th, 2010 @ 04:57 AM
- State changed from “new” to “open”
- Importance changed from “” to “”
You flagged this item as spam.
Greg Hazel December 15th, 2010 @ 05:01 AM
+1

Ran in to this (several times a second...) when trying to use Rails with config.threadsafe!
Michael Koziarski December 20th, 2010 @ 02:37 AM
I'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?
You flagged this item as spam.
Matt D February 9th, 2011 @ 10:31 PM
Running into this issue regularly in production. +1