From bd31aec9c33453e0ba96aec614e56958784e6b8d Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Mon, 20 Jul 2015 08:09:21 +0930 Subject: We need stricter locking before we can unload Specifically, the "loose upgrades" behaviour that allows us to obtain an exclusive right to load things while other requests are in progress (but waiting on the exclusive lock for themselves) prevents us from treating load & unload interchangeably: new things appearing is fine, but they do *not* expect previously-present constants to vanish. We can still use loose upgrades for unloading -- once someone has decided to unload, they don't really care if someone else gets there first -- it just needs to be tracked separately. --- .../lib/active_support/concurrency/share_lock.rb | 56 +++++++++++----------- activesupport/lib/active_support/dependencies.rb | 9 +++- .../lib/active_support/dependencies/interlock.rb | 16 +++++-- 3 files changed, 48 insertions(+), 33 deletions(-) (limited to 'activesupport') diff --git a/activesupport/lib/active_support/concurrency/share_lock.rb b/activesupport/lib/active_support/concurrency/share_lock.rb index f1c6230084..39ae9bfb79 100644 --- a/activesupport/lib/active_support/concurrency/share_lock.rb +++ b/activesupport/lib/active_support/concurrency/share_lock.rb @@ -9,7 +9,7 @@ module ActiveSupport #-- # Note that a pending Exclusive lock attempt does not block incoming # Share requests (i.e., we are "read-preferring"). That seems - # consistent with the behavior of +loose_upgrades+, but may be the + # consistent with the behavior of "loose" upgrades, but may be the # wrong choice otherwise: it nominally reduces the possibility of # deadlock by risking starvation instead. class ShareLock @@ -20,47 +20,46 @@ module ActiveSupport # 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 - + def initialize super() @cv = new_cond @sharing = Hash.new(0) + @waiting = {} @exclusive_thread = nil @exclusive_depth = 0 end - # Returns false if +no_wait+ is specified and the lock is not + # Returns false if +no_wait+ is set and the lock is not # immediately available. Otherwise, returns true after the lock # has been acquired. - def start_exclusive(no_wait=false) + # + # +purpose+ and +compatible+ work together; while this thread is + # waiting for the exclusive lock, it will yield its share (if any) + # to any other attempt whose +purpose+ appears in this attempt's + # +compatible+ list. This allows a "loose" upgrade, which, being + # less strict, 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. With + # +purpose+ matching, it is possible to yield only to other + # threads whose activity will not interfere. + def start_exclusive(purpose: nil, compatible: [], no_wait: false) synchronize do unless @exclusive_thread == Thread.current - return false if no_wait && busy? + if busy?(purpose) + return false if no_wait - loose_shares = nil - if @loose_upgrades loose_shares = @sharing.delete(Thread.current) - end + @waiting[Thread.current] = compatible if loose_shares - @cv.wait_while { busy? } if busy? + @cv.wait_while { busy?(purpose) } + @waiting.delete Thread.current + @sharing[Thread.current] = loose_shares if loose_shares + end @exclusive_thread = Thread.current - @sharing[Thread.current] = loose_shares if loose_shares end @exclusive_depth += 1 @@ -106,8 +105,10 @@ module ActiveSupport # +no_wait+ is set and the lock is not immediately available, # returns +nil+ without yielding. Otherwise, returns the result of # the block. - def exclusive(no_wait=false) - if start_exclusive(no_wait) + # + # See +start_exclusive+ for other options. + def exclusive(purpose: nil, compatible: [], no_wait: false) + if start_exclusive(purpose: purpose, compatible: compatible, no_wait: no_wait) begin yield ensure @@ -129,8 +130,9 @@ module ActiveSupport private # Must be called within synchronize - def busy? + def busy?(purpose) (@exclusive_thread && @exclusive_thread != Thread.current) || + @waiting.any? { |k, v| k != Thread.current && !v.include?(purpose) } || @sharing.size > (@sharing[Thread.current] > 0 ? 1 : 0) end end diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb index fc6f822969..18555fbcba 100644 --- a/activesupport/lib/active_support/dependencies.rb +++ b/activesupport/lib/active_support/dependencies.rb @@ -37,6 +37,13 @@ module ActiveSupport #:nodoc: Dependencies.interlock.loading { yield } end + # Execute the supplied block while holding an exclusive lock, + # preventing any other thread from being inside a #run_interlock + # block at the same time + def self.unload_interlock + Dependencies.interlock.unloading { yield } + end + # :nodoc: # Should we turn on Ruby warnings on the first load of dependent files? @@ -348,7 +355,7 @@ module ActiveSupport #:nodoc: def clear log_call - Dependencies.load_interlock do + Dependencies.unload_interlock do loaded.clear loading.clear remove_unloadable_constants! diff --git a/activesupport/lib/active_support/dependencies/interlock.rb b/activesupport/lib/active_support/dependencies/interlock.rb index 148212c951..fbeb904684 100644 --- a/activesupport/lib/active_support/dependencies/interlock.rb +++ b/activesupport/lib/active_support/dependencies/interlock.rb @@ -4,21 +4,27 @@ module ActiveSupport #:nodoc: module Dependencies #:nodoc: class Interlock def initialize # :nodoc: - @lock = ActiveSupport::Concurrency::ShareLock.new(true) + @lock = ActiveSupport::Concurrency::ShareLock.new end def loading - @lock.exclusive do + @lock.exclusive(purpose: :load, compatible: [:load]) do yield end end - # Attempt to obtain a "loading" (exclusive) lock. If possible, + def unloading + @lock.exclusive(purpose: :unload, compatible: [:load, :unload]) do + yield + end + end + + # Attempt to obtain an "unloading" (exclusive) lock. If possible, # execute the supplied block while holding the lock. If there is # concurrent activity, return immediately (without executing the # block) instead of waiting. - def attempt_loading - @lock.exclusive(true) do + def attempt_unloading + @lock.exclusive(purpose: :unload, compatible: [:load, :unload], no_wait: true) do yield end end -- cgit v1.2.3