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 +++++++++++++++------- .../lib/active_support/dependencies/interlock.rb | 6 +++ activesupport/test/share_lock_test.rb | 26 ++++++++++++ 3 files changed, 65 insertions(+), 15 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 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 diff --git a/activesupport/lib/active_support/dependencies/interlock.rb b/activesupport/lib/active_support/dependencies/interlock.rb index b6a1b25eee..47bcecbb35 100644 --- a/activesupport/lib/active_support/dependencies/interlock.rb +++ b/activesupport/lib/active_support/dependencies/interlock.rb @@ -42,6 +42,12 @@ module ActiveSupport #:nodoc: yield end end + + def permit_concurrent_loads + @lock.yield_shares(compatible: [:load]) do + yield + end + end end end end diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index 12953d99a6..68fa5bb69e 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -287,6 +287,32 @@ class ShareLockTest < ActiveSupport::TestCase assert_threads_not_stuck threads end + def test_manual_yield + ready = Concurrent::CyclicBarrier.new(2) + done = Concurrent::CyclicBarrier.new(2) + + threads = [ + Thread.new do + @lock.sharing do + ready.wait + @lock.exclusive(purpose: :x) {} + done.wait + end + end, + + Thread.new do + @lock.sharing do + ready.wait + @lock.yield_shares(compatible: [:x]) do + done.wait + end + end + end, + ] + + assert_threads_not_stuck threads + end + def test_in_shared_section_incompatible_non_upgrading_threads_cannot_preempt_upgrading_threads scratch_pad = [] scratch_pad_mutex = Mutex.new -- cgit v1.2.3 From 9f2df7856d0515f097d517395f944e6faf414d6d Mon Sep 17 00:00:00 2001 From: thedarkone Date: Sun, 7 Feb 2016 00:57:37 +0100 Subject: AS::Conc::ShareLock#yield_shares tests. --- activesupport/test/share_lock_test.rb | 107 ++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) (limited to 'activesupport') diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index 68fa5bb69e..5fceebf023 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -313,6 +313,113 @@ class ShareLockTest < ActiveSupport::TestCase assert_threads_not_stuck threads end + def test_manual_incompatible_yield + ready = Concurrent::CyclicBarrier.new(2) + done = Concurrent::CyclicBarrier.new(2) + + threads = [ + Thread.new do + @lock.sharing do + ready.wait + @lock.exclusive(purpose: :x) {} + done.wait + end + end, + + Thread.new do + @lock.sharing do + ready.wait + @lock.yield_shares(compatible: [:y]) do + done.wait + end + end + end, + ] + + assert_threads_stuck threads + ensure + threads.each(&:kill) if threads + end + + def test_manual_recursive_yield + ready = Concurrent::CyclicBarrier.new(2) + done = Concurrent::CyclicBarrier.new(2) + do_compatible_nesting = Concurrent::CountDownLatch.new + + threads = [ + Thread.new do + @lock.sharing do + ready.wait + @lock.exclusive(purpose: :x) {} + done.wait + end + end, + + Thread.new do + @lock.sharing do + ready.wait + @lock.yield_shares(compatible: [:y]) do + do_compatible_nesting.wait + @lock.sharing do + @lock.yield_shares(compatible: [:x, :y]) do + done.wait + end + end + end + end + end + ] + + assert_threads_stuck threads + do_compatible_nesting.count_down + assert_threads_not_stuck threads + end + + def test_manual_recursive_yield_restores_previous_compatible + ready = Concurrent::CyclicBarrier.new(2) + do_nesting = Concurrent::CountDownLatch.new + after_nesting = Concurrent::CountDownLatch.new + + incompatible_thread = Thread.new do + ready.wait + @lock.exclusive(purpose: :z) {} + end + + recursive_yield_shares_thread = Thread.new do + @lock.sharing do + ready.wait + @lock.yield_shares(compatible: [:y]) do + do_nesting.wait + @lock.sharing do + @lock.yield_shares(compatible: [:x, :y]) {} + end + after_nesting.wait + end + end + end + + assert_threads_stuck incompatible_thread + do_nesting.count_down + assert_threads_stuck incompatible_thread + + compatible_thread = Thread.new do + @lock.exclusive(purpose: :y) {} + end + assert_threads_not_stuck compatible_thread + + post_nesting_incompatible_thread = Thread.new do + @lock.exclusive(purpose: :x) {} + end + assert_threads_stuck post_nesting_incompatible_thread + + after_nesting.count_down + assert_threads_not_stuck recursive_yield_shares_thread + # post_nesting_incompatible_thread can now proceed + assert_threads_not_stuck post_nesting_incompatible_thread + # assert_threads_not_stuck can now proceed + assert_threads_not_stuck incompatible_thread + end + def test_in_shared_section_incompatible_non_upgrading_threads_cannot_preempt_upgrading_threads scratch_pad = [] scratch_pad_mutex = Mutex.new -- 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') 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 5d9e59131312f83f925e2ce59865bcb537976422 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Sun, 7 Feb 2016 19:02:37 +0100 Subject: Fix a nonsensical ShareLock test. --- activesupport/test/share_lock_test.rb | 43 +++++++++++++++++------------------ 1 file changed, 21 insertions(+), 22 deletions(-) (limited to 'activesupport') diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index 5fceebf023..143e65aa0c 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -341,38 +341,37 @@ class ShareLockTest < ActiveSupport::TestCase threads.each(&:kill) if threads end - def test_manual_recursive_yield + def test_manual_recursive_yield_cannot_expand_outer_compatible ready = Concurrent::CyclicBarrier.new(2) - done = Concurrent::CyclicBarrier.new(2) do_compatible_nesting = Concurrent::CountDownLatch.new + in_compatible_nesting = Concurrent::CountDownLatch.new - threads = [ - Thread.new do - @lock.sharing do - ready.wait - @lock.exclusive(purpose: :x) {} - done.wait - end - end, + incompatible_thread = Thread.new do + @lock.sharing do + ready.wait + @lock.exclusive(purpose: :x) {} + end + end - Thread.new do - @lock.sharing do - ready.wait - @lock.yield_shares(compatible: [:y]) do - do_compatible_nesting.wait - @lock.sharing do - @lock.yield_shares(compatible: [:x, :y]) do - done.wait - end + yield_shares_thread = Thread.new do + @lock.sharing do + ready.wait + @lock.yield_shares(compatible: [:y]) do + do_compatible_nesting.wait + @lock.sharing do + @lock.yield_shares(compatible: [:x, :y]) do + in_compatible_nesting.wait end end end end - ] + end - assert_threads_stuck threads + assert_threads_stuck incompatible_thread do_compatible_nesting.count_down - assert_threads_not_stuck threads + assert_threads_stuck incompatible_thread + in_compatible_nesting.count_down + assert_threads_not_stuck [yield_shares_thread, incompatible_thread] end def test_manual_recursive_yield_restores_previous_compatible -- cgit v1.2.3 From fe7d77cc01ae652a15b2c1896f677a56133bc0f1 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Mon, 8 Feb 2016 05:13:05 +1030 Subject: Test the happy path for recursive yields too --- activesupport/test/share_lock_test.rb | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) (limited to 'activesupport') diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index 143e65aa0c..acefa185a8 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -341,6 +341,41 @@ class ShareLockTest < ActiveSupport::TestCase threads.each(&:kill) if threads end + def test_manual_recursive_yield + ready = Concurrent::CyclicBarrier.new(2) + done = Concurrent::CyclicBarrier.new(2) + do_nesting = Concurrent::CountDownLatch.new + + threads = [ + Thread.new do + @lock.sharing do + ready.wait + @lock.exclusive(purpose: :x) {} + done.wait + end + end, + + Thread.new do + @lock.sharing do + @lock.yield_shares(compatible: [:x]) do + @lock.sharing do + ready.wait + do_nesting.wait + @lock.yield_shares(compatible: [:x, :y]) do + done.wait + end + end + end + end + end + ] + + assert_threads_stuck threads + do_nesting.count_down + + assert_threads_not_stuck threads + end + def test_manual_recursive_yield_cannot_expand_outer_compatible ready = Concurrent::CyclicBarrier.new(2) do_compatible_nesting = Concurrent::CountDownLatch.new -- 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') 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