diff options
| -rw-r--r-- | actionpack/lib/action_dispatch.rb | 1 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/middleware/load_interlock.rb | 12 | ||||
| -rw-r--r-- | activesupport/lib/active_support/concurrency/share_lock.rb | 118 | ||||
| -rw-r--r-- | activesupport/lib/active_support/dependencies.rb | 81 | ||||
| -rw-r--r-- | activesupport/lib/active_support/dependencies/interlock.rb | 35 | ||||
| -rw-r--r-- | railties/lib/rails/application/default_middleware_stack.rb | 38 | ||||
| -rw-r--r-- | railties/test/application/middleware_test.rb | 25 |
7 files changed, 263 insertions, 47 deletions
diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb index dcd3ee0644..f6336c8c7a 100644 --- a/actionpack/lib/action_dispatch.rb +++ b/actionpack/lib/action_dispatch.rb @@ -52,6 +52,7 @@ module ActionDispatch autoload :DebugExceptions autoload :ExceptionWrapper autoload :Flash + autoload :LoadInterlock autoload :ParamsParser autoload :PublicExceptions autoload :Reloader diff --git a/actionpack/lib/action_dispatch/middleware/load_interlock.rb b/actionpack/lib/action_dispatch/middleware/load_interlock.rb new file mode 100644 index 0000000000..cbe8d750fe --- /dev/null +++ b/actionpack/lib/action_dispatch/middleware/load_interlock.rb @@ -0,0 +1,12 @@ +require 'active_support/dependencies' +require 'rack/lock' + +module ActionDispatch + class LoadInterlock < ::Rack::Lock + FLAG = 'activesupport.dependency_race'.freeze + + def initialize(app, mutex = ::ActiveSupport::Dependencies.interlock) + super + end + end +end diff --git a/activesupport/lib/active_support/concurrency/share_lock.rb b/activesupport/lib/active_support/concurrency/share_lock.rb new file mode 100644 index 0000000000..e03f2cfc7a --- /dev/null +++ b/activesupport/lib/active_support/concurrency/share_lock.rb @@ -0,0 +1,118 @@ +require 'thread' +require 'monitor' + +module ActiveSupport + module Concurrency + class ShareLock + include MonitorMixin + + # We track Thread objects, instead of just using counters, because + # we need exclusive locks to be reentrant, and we need to be able + # to upgrade share locks to exclusive. + + + # If +loose_upgrades+ is false (the default), then a thread that + # is waiting on an Exclusive lock will continue to hold any Share + # lock that it has already established. This is safer, but can + # lead to deadlock. + # + # If +loose_upgrades+ is true, a thread waiting on an Exclusive + # lock will temporarily relinquish its Share lock. Being less + # strict, this behavior prevents some classes of deadlocks. For + # many resources, loose upgrades are sufficient: if a thread is + # awaiting a lock, it is not running any other code. + attr_reader :loose_upgrades + + def initialize(loose_upgrades = false) + @loose_upgrades = loose_upgrades + + super() + + @cv = new_cond + + @sharing = Hash.new(0) + @exclusive_thread = nil + @exclusive_depth = 0 + end + + def start_exclusive(no_wait=false) + synchronize do + unless @exclusive_thread == Thread.current + return false if no_wait && busy? + + loose_shares = nil + if @loose_upgrades + loose_shares = @sharing.delete(Thread.current) + end + + @cv.wait_while { busy? } if busy? + + @exclusive_thread = Thread.current + @sharing[Thread.current] = loose_shares if loose_shares + end + @exclusive_depth += 1 + + true + end + end + + def stop_exclusive + synchronize do + raise "invalid unlock" if @exclusive_thread != Thread.current + + @exclusive_depth -= 1 + if @exclusive_depth == 0 + @exclusive_thread = nil + @cv.broadcast + end + end + end + + def start_sharing + synchronize do + if @exclusive_thread && @exclusive_thread != Thread.current + @cv.wait_while { @exclusive_thread } + end + @sharing[Thread.current] += 1 + end + end + + def stop_sharing + synchronize do + if @sharing[Thread.current] > 1 + @sharing[Thread.current] -= 1 + else + @sharing.delete Thread.current + @cv.broadcast + end + end + end + + def exclusive(no_wait=false) + if start_exclusive(no_wait) + begin + yield + ensure + stop_exclusive + end + end + end + + def sharing + start_sharing + begin + yield + ensure + stop_sharing + end + end + + private + + def busy? + (@exclusive_thread && @exclusive_thread != Thread.current) || + @sharing.size > (@sharing[Thread.current] > 0 ? 1 : 0) + end + end + end +end diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb index 664cc15a29..2a0e06495f 100644 --- a/activesupport/lib/active_support/dependencies.rb +++ b/activesupport/lib/active_support/dependencies.rb @@ -12,12 +12,16 @@ require 'active_support/core_ext/kernel/reporting' require 'active_support/core_ext/load_error' require 'active_support/core_ext/name_error' require 'active_support/core_ext/string/starts_ends_with' +require "active_support/dependencies/interlock" require 'active_support/inflector' module ActiveSupport #:nodoc: module Dependencies #:nodoc: extend self + mattr_accessor :interlock + self.interlock = Interlock.new + # Should we turn on Ruby warnings on the first load of dependent files? mattr_accessor :warnings_on_first_load self.warnings_on_first_load = false @@ -234,10 +238,12 @@ module ActiveSupport #:nodoc: end def load_dependency(file) - if Dependencies.load? && ActiveSupport::Dependencies.constant_watch_stack.watching? - Dependencies.new_constants_in(Object) { yield } - else - yield + Dependencies.interlock.loading do + if Dependencies.load? && ActiveSupport::Dependencies.constant_watch_stack.watching? + Dependencies.new_constants_in(Object) { yield } + else + yield + end end rescue Exception => exception # errors from loading file exception.blame_file! file if exception.respond_to? :blame_file! @@ -325,9 +331,11 @@ module ActiveSupport #:nodoc: def clear log_call - loaded.clear - loading.clear - remove_unloadable_constants! + Dependencies.interlock.loading do + loaded.clear + loading.clear + remove_unloadable_constants! + end end def require_or_load(file_name, const_path = nil) @@ -336,39 +344,44 @@ module ActiveSupport #:nodoc: expanded = File.expand_path(file_name) return if loaded.include?(expanded) - # Record that we've seen this file *before* loading it to avoid an - # infinite loop with mutual dependencies. - loaded << expanded - loading << expanded - - begin - if load? - log "loading #{file_name}" + Dependencies.interlock.loading do + # Maybe it got loaded while we were waiting for our lock: + return if loaded.include?(expanded) - # Enable warnings if this file has not been loaded before and - # warnings_on_first_load is set. - load_args = ["#{file_name}.rb"] - load_args << const_path unless const_path.nil? + # Record that we've seen this file *before* loading it to avoid an + # infinite loop with mutual dependencies. + loaded << expanded + loading << expanded - if !warnings_on_first_load or history.include?(expanded) - result = load_file(*load_args) + begin + if load? + log "loading #{file_name}" + + # Enable warnings if this file has not been loaded before and + # warnings_on_first_load is set. + load_args = ["#{file_name}.rb"] + load_args << const_path unless const_path.nil? + + if !warnings_on_first_load or history.include?(expanded) + result = load_file(*load_args) + else + enable_warnings { result = load_file(*load_args) } + end else - enable_warnings { result = load_file(*load_args) } + log "requiring #{file_name}" + result = require file_name end - else - log "requiring #{file_name}" - result = require file_name + rescue Exception + loaded.delete expanded + raise + ensure + loading.pop end - rescue Exception - loaded.delete expanded - raise - ensure - loading.pop - end - # Record history *after* loading so first load gets warnings. - history << expanded - result + # Record history *after* loading so first load gets warnings. + history << expanded + result + end end # Is the provided constant path defined? diff --git a/activesupport/lib/active_support/dependencies/interlock.rb b/activesupport/lib/active_support/dependencies/interlock.rb new file mode 100644 index 0000000000..c3601bac13 --- /dev/null +++ b/activesupport/lib/active_support/dependencies/interlock.rb @@ -0,0 +1,35 @@ +require 'active_support/concurrency/share_lock' + +module ActiveSupport + module Dependencies #:nodoc: + class Interlock + def initialize + @lock = ActiveSupport::Concurrency::ShareLock.new(true) + end + + def loading + @lock.exclusive do + yield + end + end + + def start_running + @lock.start_sharing + end + + def done_running + @lock.stop_sharing + end + + def running + @lock.sharing do + yield + end + end + + # Match the Mutex API, so we can be used by Rack::Lock + alias :lock :start_running + alias :unlock :done_running + end + end +end diff --git a/railties/lib/rails/application/default_middleware_stack.rb b/railties/lib/rails/application/default_middleware_stack.rb index 6f9ccec137..14ea073039 100644 --- a/railties/lib/rails/application/default_middleware_stack.rb +++ b/railties/lib/rails/application/default_middleware_stack.rb @@ -26,7 +26,35 @@ module Rails middleware.use ::Rack::Cache, rack_cache end - middleware.use ::Rack::Lock unless allow_concurrency? + if config.allow_concurrency == false + # User has explicitly opted out of concurrent request + # handling: presumably their code is not threadsafe + + middleware.use ::Rack::Lock + + elsif config.allow_concurrency + # Do nothing, even if we know this is dangerous + + else + # Default concurrency setting + + if config.cache_classes && config.eager_load + # No lock required + + elsif config.cache_classes + # The load interlock is required, but not a full request + # lock + + middleware.use ::ActionDispatch::LoadInterlock + + else + # If we're reloading on each request, they all need to be + # run in isolation + + middleware.use ::Rack::Lock + end + end + middleware.use ::Rack::Runtime middleware.use ::Rack::MethodOverride unless config.api_only middleware.use ::ActionDispatch::RequestId @@ -65,14 +93,6 @@ module Rails config.reload_classes_only_on_change != true || app.reloaders.map(&:updated?).any? end - def allow_concurrency? - if config.allow_concurrency.nil? - config.cache_classes && config.eager_load - else - config.allow_concurrency - end - end - def load_rack_cache rack_cache = config.action_dispatch.rack_cache return unless rack_cache diff --git a/railties/test/application/middleware_test.rb b/railties/test/application/middleware_test.rb index ce92ebbf66..0ea2c57c55 100644 --- a/railties/test/application/middleware_test.rb +++ b/railties/test/application/middleware_test.rb @@ -121,23 +121,40 @@ module ApplicationTests assert !middleware.include?("ActiveRecord::Migration::CheckPending") end - test "includes lock if cache_classes is set but eager_load is not" do + test "includes interlock if cache_classes is set but eager_load is not" do add_to_config "config.cache_classes = true" boot! - assert middleware.include?("Rack::Lock") + assert_not_includes middleware, "Rack::Lock" + assert_includes middleware, "ActionDispatch::LoadInterlock" + end + + test "includes lock if cache_classes is off" do + add_to_config "config.cache_classes = false" + boot! + assert_includes middleware, "Rack::Lock" + assert_not_includes middleware, "ActionDispatch::LoadInterlock" end test "does not include lock if cache_classes is set and so is eager_load" do add_to_config "config.cache_classes = true" add_to_config "config.eager_load = true" boot! - assert !middleware.include?("Rack::Lock") + assert_not_includes middleware, "Rack::Lock" + assert_not_includes middleware, "ActionDispatch::LoadInterlock" end test "does not include lock if allow_concurrency is set" do add_to_config "config.allow_concurrency = true" boot! - assert !middleware.include?("Rack::Lock") + assert_not_includes middleware, "Rack::Lock" + assert_not_includes middleware, "ActionDispatch::LoadInterlock" + end + + test "includes lock if allow_concurrency is disabled" do + add_to_config "config.allow_concurrency = false" + boot! + assert_includes middleware, "Rack::Lock" + assert_not_includes middleware, "ActionDispatch::LoadInterlock" end test "removes static asset server if serve_static_files is disabled" do |