Merge pull request #20928 from matthewd/unload-interlock

We need stricter locking before we can unload

3 files changed, 51 insertions, 34 deletions

diff --git a/activesupport/lib/active_support/concurrency/share_lock.rb b/activesupport/lib/active_support/concurrency/share_lock.rb
index f1c6230084..ca48164c54 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,48 @@ 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)
+              @waiting[Thread.current] = compatible if loose_shares
+
+              begin
+                @cv.wait_while { busy?(purpose) }
+              ensure
+                @waiting.delete Thread.current
+                @sharing[Thread.current] = loose_shares if loose_shares
+              end
             end
-
-            @cv.wait_while { busy? } if busy?
-
             @exclusive_thread = Thread.current
-            @sharing[Thread.current] = loose_shares if loose_shares
           end
           @exclusive_depth += 1
 
@@ -106,8 +107,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 +132,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 be03f65e6e..f76ef04f49 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?
@@ -346,7 +353,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