Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

Closed
lighthouse-import opened this Issue · 9 comments

3 participants

@lighthouse-import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/3457
Created by blythe - 2009-11-04 21:11:46 UTC

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?

@lighthouse-import

Imported from Lighthouse.
Comment by Timothy Jones - 2010-02-04 05:12:55 UTC

+1

@lighthouse-import

Imported from Lighthouse.
Comment by Michael Rykov - 2010-04-07 20:07:43 UTC

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  
@lighthouse-import

Imported from Lighthouse.
Comment by Greg Hazel - 2010-12-15 05:01:06 UTC

+1

Ran in to this (several times a second...) when trying to use Rails with config.threadsafe!

@lighthouse-import

Imported from Lighthouse.
Comment by Michael Koziarski - 2010-12-20 02:37:03 UTC

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?

@lighthouse-import

Imported from Lighthouse.
Comment by Matt D - 2011-02-09 22:31:01 UTC

Running into this issue regularly in production. +1

@jake3030 jake3030 referenced this issue from a commit in jake3030/rails
@fcheung fcheung Preload uses exclusive scope [#643 state:resolved]
With self referential associations, the scope for the the top level should not affect fetching of associations, for example
when doing

Person.male.find :all, :include => :friends

we should load all of the friends for each male, not just the male friends.
5cebe69
@djmaze

This has not yet been solved Can the issue be reopened?

@nilbus

I'm also still running into this on 2.3.14. Is this something that could still be accepted into 2-3-stable?

@nilbus

Monkey patch for 2.3.14, for those who can't wait :-)

# Fix https://github.com/rails/rails/issues/643
module ActionController::Caching
  class Sweeper < ActiveRecord::Observer
    def controller
      Thread.current["#{self.class.name}_controller"]
    end

    def controller=(controller)
      Thread.current["#{self.class.name}_controller"] = controller
    end

  private

    def method_missing(method, *arguments, &block)
      return if controller.nil?
      controller.__send__(method, *arguments, &block)
    end
  end
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.