From 3e4a69e52d8c8f0335e0ddbb46fe21009b962334 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Sun, 7 Feb 2016 08:24:57 +1030 Subject: Hand off the interlock to the new thread in AC::Live Most importantly, the original request thread must yield its share lock while waiting for the live thread to commit -- otherwise a request's base and live threads can deadlock against each other. --- .../lib/active_support/concurrency/share_lock.rb | 48 +++++++++++++++------- 1 file changed, 33 insertions(+), 15 deletions(-) (limited to 'activesupport/lib/active_support/concurrency/share_lock.rb') diff --git a/activesupport/lib/active_support/concurrency/share_lock.rb b/activesupport/lib/active_support/concurrency/share_lock.rb index 8e4ca272ba..2ac278a2f1 100644 --- a/activesupport/lib/active_support/concurrency/share_lock.rb +++ b/activesupport/lib/active_support/concurrency/share_lock.rb @@ -51,7 +51,7 @@ module ActiveSupport if busy_for_exclusive?(purpose) return false if no_wait - yield_shares(purpose, compatible) do + yield_shares(purpose: purpose, compatible: compatible) do @cv.wait_while { busy_for_exclusive?(purpose) } end end @@ -73,10 +73,10 @@ module ActiveSupport if @exclusive_depth == 0 @exclusive_thread = nil - yield_shares(nil, compatible) do - @cv.broadcast + yield_shares(compatible: compatible) do @cv.wait_while { @exclusive_thread || eligible_waiters?(compatible) } end + @cv.broadcast end end end @@ -127,6 +127,36 @@ module ActiveSupport end end + def yield_shares(purpose: nil, compatible: []) + loose_shares = previous_wait = nil + synchronize do + if loose_shares = @sharing.delete(Thread.current) + if previous_wait = @waiting[Thread.current] + purpose = nil unless purpose == previous_wait[0] + compatible &= previous_wait[1] + end + @waiting[Thread.current] = [purpose, compatible] + end + + @cv.broadcast + end + + begin + yield + ensure + synchronize do + @cv.wait_while { @exclusive_thread && @exclusive_thread != Thread.current } + + if previous_wait + @waiting[Thread.current] = previous_wait + else + @waiting.delete Thread.current + end + @sharing[Thread.current] = loose_shares if loose_shares + end + end + end + private # Must be called within synchronize @@ -143,18 +173,6 @@ module ActiveSupport def eligible_waiters?(compatible) @waiting.any? { |t, (p, _)| compatible.include?(p) && @waiting.all? { |t2, (_, c2)| t == t2 || c2.include?(p) } } end - - def yield_shares(purpose, compatible) - loose_shares = @sharing.delete(Thread.current) - @waiting[Thread.current] = [purpose, compatible] if loose_shares - - begin - yield - ensure - @waiting.delete Thread.current - @sharing[Thread.current] = loose_shares if loose_shares - end - end end end end -- cgit v1.2.3 From d2c671e3cffc60ef00df63a28cc9ae0eb89f1b31 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Mon, 8 Feb 2016 00:36:21 +1030 Subject: Eagerly reacquire when start_sharing is nested inside yield_shares A full write-preferring wait can lead to deadlock. --- .../lib/active_support/concurrency/share_lock.rb | 29 ++++++++++++++-------- 1 file changed, 18 insertions(+), 11 deletions(-) (limited to 'activesupport/lib/active_support/concurrency/share_lock.rb') diff --git a/activesupport/lib/active_support/concurrency/share_lock.rb b/activesupport/lib/active_support/concurrency/share_lock.rb index 2ac278a2f1..ed3345d4dc 100644 --- a/activesupport/lib/active_support/concurrency/share_lock.rb +++ b/activesupport/lib/active_support/concurrency/share_lock.rb @@ -6,12 +6,6 @@ module ActiveSupport # A share/exclusive lock, otherwise known as a read/write lock. # # https://en.wikipedia.org/wiki/Readers%E2%80%93writer_lock - #-- - # 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 - # wrong choice otherwise: it nominally reduces the possibility of - # deadlock by risking starvation instead. class ShareLock include MonitorMixin @@ -73,8 +67,10 @@ module ActiveSupport if @exclusive_depth == 0 @exclusive_thread = nil - yield_shares(compatible: compatible) do - @cv.wait_while { @exclusive_thread || eligible_waiters?(compatible) } + if eligible_waiters?(compatible) + yield_shares(compatible: compatible) do + @cv.wait_while { @exclusive_thread || eligible_waiters?(compatible) } + end end @cv.broadcast end @@ -83,7 +79,15 @@ module ActiveSupport def start_sharing(purpose: :share) synchronize do - if @sharing[Thread.current] == 0 && @exclusive_thread != Thread.current && busy_for_sharing?(purpose) + if @sharing[Thread.current] > 0 || @exclusive_thread == Thread.current + # We already hold a lock; nothing to wait for + elsif @waiting[Thread.current] + # We're nested inside a +yield_shares+ call: we'll resume as + # soon as there isn't an exclusive lock in our way + @cv.wait_while { @exclusive_thread } + else + # This is an initial / outermost share call: any outstanding + # requests for an exclusive lock get to go first @cv.wait_while { busy_for_sharing?(purpose) } end @sharing[Thread.current] += 1 @@ -127,6 +131,9 @@ module ActiveSupport end end + # Temporarily give up all held Share locks while executing the + # supplied block, allowing any +compatible+ exclusive lock request + # to proceed. def yield_shares(purpose: nil, compatible: []) loose_shares = previous_wait = nil synchronize do @@ -136,9 +143,9 @@ module ActiveSupport compatible &= previous_wait[1] end @waiting[Thread.current] = [purpose, compatible] - end - @cv.broadcast + @cv.broadcast + end end begin -- cgit v1.2.3 From 11579b8306dd6303b306ee10c8bc9f7a4a6adea3 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Mon, 8 Feb 2016 05:22:56 +1030 Subject: Manual yield doesn't block new shares --- activesupport/lib/active_support/concurrency/share_lock.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) (limited to 'activesupport/lib/active_support/concurrency/share_lock.rb') diff --git a/activesupport/lib/active_support/concurrency/share_lock.rb b/activesupport/lib/active_support/concurrency/share_lock.rb index ed3345d4dc..54244317e4 100644 --- a/activesupport/lib/active_support/concurrency/share_lock.rb +++ b/activesupport/lib/active_support/concurrency/share_lock.rb @@ -45,7 +45,7 @@ module ActiveSupport if busy_for_exclusive?(purpose) return false if no_wait - yield_shares(purpose: purpose, compatible: compatible) do + yield_shares(purpose: purpose, compatible: compatible, block_share: true) do @cv.wait_while { busy_for_exclusive?(purpose) } end end @@ -68,7 +68,7 @@ module ActiveSupport @exclusive_thread = nil if eligible_waiters?(compatible) - yield_shares(compatible: compatible) do + yield_shares(compatible: compatible, block_share: true) do @cv.wait_while { @exclusive_thread || eligible_waiters?(compatible) } end end @@ -77,7 +77,7 @@ module ActiveSupport end end - def start_sharing(purpose: :share) + def start_sharing synchronize do if @sharing[Thread.current] > 0 || @exclusive_thread == Thread.current # We already hold a lock; nothing to wait for @@ -88,7 +88,7 @@ module ActiveSupport else # This is an initial / outermost share call: any outstanding # requests for an exclusive lock get to go first - @cv.wait_while { busy_for_sharing?(purpose) } + @cv.wait_while { busy_for_sharing?(false) } end @sharing[Thread.current] += 1 end @@ -134,7 +134,7 @@ module ActiveSupport # Temporarily give up all held Share locks while executing the # supplied block, allowing any +compatible+ exclusive lock request # to proceed. - def yield_shares(purpose: nil, compatible: []) + def yield_shares(purpose: nil, compatible: [], block_share: false) loose_shares = previous_wait = nil synchronize do if loose_shares = @sharing.delete(Thread.current) @@ -142,6 +142,7 @@ module ActiveSupport purpose = nil unless purpose == previous_wait[0] compatible &= previous_wait[1] end + compatible |= [false] unless block_share @waiting[Thread.current] = [purpose, compatible] @cv.broadcast -- cgit v1.2.3