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 From 9c4da24aca20633fa109c92cbeee71ebd5ea2e87 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Mon, 20 Jul 2015 18:58:20 +0200 Subject: Tests for AS::Concurrency::ShareLock. --- activesupport/test/share_lock_test.rb | 194 ++++++++++++++++++++++++++++++++++ 1 file changed, 194 insertions(+) create mode 100644 activesupport/test/share_lock_test.rb (limited to 'activesupport') diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb new file mode 100644 index 0000000000..fd50516581 --- /dev/null +++ b/activesupport/test/share_lock_test.rb @@ -0,0 +1,194 @@ +require 'abstract_unit' +require 'concurrent/atomics' +require 'active_support/concurrency/share_lock' + +class ShareLockTest < ActiveSupport::TestCase + def setup + @lock = ActiveSupport::Concurrency::ShareLock.new + end + + def test_sharing_doesnt_block + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_latch| + assert_threads_not_stuck(Thread.new {@lock.sharing {} }) + end + end + + def test_sharing_blocks_exclusive + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + @lock.exclusive(no_wait: true) { flunk } # polling should fail + exclusive_thread = Thread.new { @lock.exclusive {} } + assert_threads_stuck_but_releasable_by_latch exclusive_thread, sharing_thread_release_latch + end + end + + def test_exclusive_blocks_sharing + with_thread_waiting_in_lock_section(:exclusive) do |exclusive_thread_release_latch| + sharing_thread = Thread.new { @lock.sharing {} } + assert_threads_stuck_but_releasable_by_latch sharing_thread, exclusive_thread_release_latch + end + end + + def test_multiple_exlusives_are_able_to_progress + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + exclusive_threads = (1..2).map do + Thread.new do + @lock.exclusive {} + end + end + + assert_threads_stuck_but_releasable_by_latch exclusive_threads, sharing_thread_release_latch + end + end + + def test_sharing_is_upgradeable_to_exclusive + upgrading_thread = Thread.new do + @lock.sharing do + @lock.exclusive {} + end + end + assert_threads_not_stuck upgrading_thread + end + + def test_exclusive_upgrade_waits_for_other_sharers_to_leave + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + in_sharing = Concurrent::CountDownLatch.new + + upgrading_thread = Thread.new do + @lock.sharing do + in_sharing.count_down + @lock.exclusive {} + end + end + + in_sharing.wait + assert_threads_stuck_but_releasable_by_latch upgrading_thread, sharing_thread_release_latch + end + end + + def test_exclusive_matching_purpose + [true, false].each do |use_upgrading| + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + exclusive_threads = (1..2).map do + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + @lock.exclusive(purpose: :load, compatible: [:load, :unload]) {} + end + end + end + + assert_threads_stuck_but_releasable_by_latch exclusive_threads, sharing_thread_release_latch + end + end + end + + def test_exclusive_conflicting_purpose + [true, false].each do |use_upgrading| + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + begin + conflicting_exclusive_threads = [ + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + @lock.exclusive(purpose: :load, compatible: [:load]) {} + end + end, + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + @lock.exclusive(purpose: :unload, compatible: [:unload]) {} + end + end + ] + + assert_threads_stuck conflicting_exclusive_threads # wait for threads to get into their respective `exclusive {}` blocks + sharing_thread_release_latch.count_down + assert_threads_stuck conflicting_exclusive_threads # assert they are stuck + + no_purpose_thread = Thread.new do + @lock.exclusive {} + end + assert_threads_not_stuck no_purpose_thread # no purpose thread is able to squeak through + + compatible_thread = Thread.new do + @lock.exclusive(purpose: :load, compatible: [:load, :unload]) + end + + assert_threads_not_stuck compatible_thread # compatible thread is able to squeak through + assert_threads_stuck conflicting_exclusive_threads # assert other threads are still stuck + ensure + conflicting_exclusive_threads.each(&:kill) + end + end + end + end + + def test_exclusive_ordering + [true, false].each do |use_upgrading| + scratch_pad = [] + scratch_pad_mutex = Mutex.new + + load_params = [:load, [:load]] + unload_params = [:unload, [:unload, :load]] + + [load_params, load_params, unload_params, unload_params].permutation do |thread_params| + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + threads = thread_params.map do |purpose, compatible| + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + @lock.exclusive(purpose: purpose, compatible: compatible) do + scratch_pad_mutex.synchronize { scratch_pad << purpose } + end + end + end + end + + sleep(0.01) + scratch_pad_mutex.synchronize { assert_empty scratch_pad } + + sharing_thread_release_latch.count_down + + assert_threads_not_stuck threads + scratch_pad_mutex.synchronize do + assert_equal [:load, :load, :unload, :unload], scratch_pad + scratch_pad.clear + end + end + end + end + end + + private + SUFFICIENT_TIMEOUT = 0.2 + + def assert_threads_stuck_but_releasable_by_latch(threads, latch) + assert_threads_stuck threads + latch.count_down + assert_threads_not_stuck threads + end + + def assert_threads_stuck(threads) + sleep(SUFFICIENT_TIMEOUT) # give threads time to do their business + assert(Array(threads).all? {|t| t.join(0.001).nil?}) + end + + def assert_threads_not_stuck(threads) + assert_not_nil(Array(threads).all? {|t| t.join(SUFFICIENT_TIMEOUT)}) + end + + def with_thread_waiting_in_lock_section(lock_section) + in_section = Concurrent::CountDownLatch.new + section_release = Concurrent::CountDownLatch.new + + stuck_thread = Thread.new do + @lock.send(lock_section) do + in_section.count_down + section_release.wait + end + end + + in_section.wait + + yield section_release + ensure + section_release.count_down + stuck_thread.join # clean up + end +end -- cgit v1.2.3 From b3d78e8c23e3cbd5325719474efb9f7ee4168f72 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Mon, 20 Jul 2015 18:59:18 +0200 Subject: Fix ShareLock issues. --- activesupport/lib/active_support/concurrency/share_lock.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 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 39ae9bfb79..35193ea9f7 100644 --- a/activesupport/lib/active_support/concurrency/share_lock.rb +++ b/activesupport/lib/active_support/concurrency/share_lock.rb @@ -52,7 +52,7 @@ module ActiveSupport return false if no_wait loose_shares = @sharing.delete(Thread.current) - @waiting[Thread.current] = compatible if loose_shares + @waiting[Thread.current] = compatible @cv.wait_while { busy?(purpose) } @@ -132,7 +132,7 @@ module ActiveSupport # Must be called within synchronize def busy?(purpose) (@exclusive_thread && @exclusive_thread != Thread.current) || - @waiting.any? { |k, v| k != Thread.current && !v.include?(purpose) } || + (purpose && @waiting.any? { |k, v| k != Thread.current && !v.include?(purpose) }) || @sharing.size > (@sharing[Thread.current] > 0 ? 1 : 0) end end -- cgit v1.2.3 From 649d8173c31ab0b74ef359e692eadf46b5da911d Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Tue, 21 Jul 2015 08:50:50 +0930 Subject: Order of execution is only guaranteed if upgrading If the thread isn't yet holding any form of lock, it has no claim over what may / may not run while it's blocked. --- activesupport/lib/active_support/concurrency/share_lock.rb | 2 +- activesupport/test/share_lock_test.rb | 4 +++- 2 files changed, 4 insertions(+), 2 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 35193ea9f7..48edcfdaa5 100644 --- a/activesupport/lib/active_support/concurrency/share_lock.rb +++ b/activesupport/lib/active_support/concurrency/share_lock.rb @@ -52,7 +52,7 @@ module ActiveSupport return false if no_wait loose_shares = @sharing.delete(Thread.current) - @waiting[Thread.current] = compatible + @waiting[Thread.current] = compatible if loose_shares @cv.wait_while { busy?(purpose) } diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index fd50516581..87cd116429 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -147,7 +147,9 @@ class ShareLockTest < ActiveSupport::TestCase assert_threads_not_stuck threads scratch_pad_mutex.synchronize do - assert_equal [:load, :load, :unload, :unload], scratch_pad + if use_upgrading + assert_equal [:load, :load, :unload, :unload], scratch_pad + end scratch_pad.clear end end -- cgit v1.2.3 From ef4d3342728ad29a8ecf0f4eebf39e4c91aaabee Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Tue, 21 Jul 2015 10:55:54 +0930 Subject: Add some meta-assertions for the custom assertions I accidentally discovered `assert_threads_not_stuck` couldn't fail, so the simplest solution was to prove they're all now working in both directions. --- activesupport/test/share_lock_test.rb | 74 +++++++++++++++++++++++++++++------ 1 file changed, 63 insertions(+), 11 deletions(-) (limited to 'activesupport') diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index 87cd116429..8ca2a46a6c 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -158,23 +158,75 @@ class ShareLockTest < ActiveSupport::TestCase end private - SUFFICIENT_TIMEOUT = 0.2 - def assert_threads_stuck_but_releasable_by_latch(threads, latch) - assert_threads_stuck threads - latch.count_down - assert_threads_not_stuck threads - end + module CustomAssertions + SUFFICIENT_TIMEOUT = 0.2 + + private + + def assert_threads_stuck_but_releasable_by_latch(threads, latch) + assert_threads_stuck threads + latch.count_down + assert_threads_not_stuck threads + end - def assert_threads_stuck(threads) - sleep(SUFFICIENT_TIMEOUT) # give threads time to do their business - assert(Array(threads).all? {|t| t.join(0.001).nil?}) + def assert_threads_stuck(threads) + sleep(SUFFICIENT_TIMEOUT) # give threads time to do their business + assert(Array(threads).all? { |t| t.join(0.001).nil? }) + end + + def assert_threads_not_stuck(threads) + assert(Array(threads).all? { |t| t.join(SUFFICIENT_TIMEOUT) }) + end end - def assert_threads_not_stuck(threads) - assert_not_nil(Array(threads).all? {|t| t.join(SUFFICIENT_TIMEOUT)}) + class CustomAssertionsTest < ActiveSupport::TestCase + include CustomAssertions + + def setup + @latch = Concurrent::CountDownLatch.new + @thread = Thread.new { @latch.wait } + end + + def teardown + @latch.count_down + @thread.join + end + + def test_happy_path + assert_threads_stuck_but_releasable_by_latch @thread, @latch + end + + def test_detects_stuck_thread + assert_raises(Minitest::Assertion) do + assert_threads_not_stuck @thread + end + end + + def test_detects_free_thread + @latch.count_down + assert_raises(Minitest::Assertion) do + assert_threads_stuck @thread + end + end + + def test_detects_already_released + @latch.count_down + assert_raises(Minitest::Assertion) do + assert_threads_stuck_but_releasable_by_latch @thread, @latch + end + end + + def test_detects_remains_latched + another_latch = Concurrent::CountDownLatch.new + assert_raises(Minitest::Assertion) do + assert_threads_stuck_but_releasable_by_latch @thread, another_latch + end + end end + include CustomAssertions + def with_thread_waiting_in_lock_section(lock_section) in_section = Concurrent::CountDownLatch.new section_release = Concurrent::CountDownLatch.new -- cgit v1.2.3 From 4c54b2a9a012296709de5283eada03470d581dc9 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Tue, 21 Jul 2015 11:47:16 +0930 Subject: Adjust expectations around purpose/compatibility options --- .../lib/active_support/concurrency/share_lock.rb | 2 +- activesupport/test/share_lock_test.rb | 43 ++++++++++++++++++---- 2 files changed, 36 insertions(+), 9 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 48edcfdaa5..39ae9bfb79 100644 --- a/activesupport/lib/active_support/concurrency/share_lock.rb +++ b/activesupport/lib/active_support/concurrency/share_lock.rb @@ -132,7 +132,7 @@ module ActiveSupport # Must be called within synchronize def busy?(purpose) (@exclusive_thread && @exclusive_thread != Thread.current) || - (purpose && @waiting.any? { |k, v| k != Thread.current && !v.include?(purpose) }) || + @waiting.any? { |k, v| k != Thread.current && !v.include?(purpose) } || @sharing.size > (@sharing[Thread.current] > 0 ? 1 : 0) end end diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index 8ca2a46a6c..efd840be79 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -88,33 +88,60 @@ class ShareLockTest < ActiveSupport::TestCase conflicting_exclusive_threads = [ Thread.new do @lock.send(use_upgrading ? :sharing : :tap) do - @lock.exclusive(purpose: :load, compatible: [:load]) {} + @lock.exclusive(purpose: :red, compatible: [:green, :purple]) {} end end, Thread.new do @lock.send(use_upgrading ? :sharing : :tap) do - @lock.exclusive(purpose: :unload, compatible: [:unload]) {} + @lock.exclusive(purpose: :blue, compatible: [:green]) {} end end ] assert_threads_stuck conflicting_exclusive_threads # wait for threads to get into their respective `exclusive {}` blocks - sharing_thread_release_latch.count_down - assert_threads_stuck conflicting_exclusive_threads # assert they are stuck + # This thread will be stuck as long as any other thread is in + # a sharing block. While it's blocked, it holds no lock, so it + # doesn't interfere with any other attempts. no_purpose_thread = Thread.new do @lock.exclusive {} end - assert_threads_not_stuck no_purpose_thread # no purpose thread is able to squeak through + assert_threads_stuck no_purpose_thread + # This thread is compatible with both of the "primary" + # attempts above. It's initially stuck on the outer share + # lock, but as soon as that's released, it can run -- + # regardless of whether those threads hold share locks. compatible_thread = Thread.new do - @lock.exclusive(purpose: :load, compatible: [:load, :unload]) + @lock.exclusive(purpose: :green, compatible: []) {} end + assert_threads_stuck compatible_thread + + assert_threads_stuck conflicting_exclusive_threads + + sharing_thread_release_latch.count_down - assert_threads_not_stuck compatible_thread # compatible thread is able to squeak through - assert_threads_stuck conflicting_exclusive_threads # assert other threads are still stuck + assert_threads_not_stuck compatible_thread # compatible thread is now able to squeak through + + if use_upgrading + # The "primary" threads both each hold a share lock, and are + # mutually incompatible; they're still stuck. + assert_threads_stuck conflicting_exclusive_threads + + # The thread without a specified purpose is also stuck; it's + # not compatible with anything. + assert_threads_stuck no_purpose_thread + else + # As the primaries didn't hold a share lock, as soon as the + # outer one was released, all the exclusive locks are free + # to be acquired in turn. + + assert_threads_not_stuck conflicting_exclusive_threads + assert_threads_not_stuck no_purpose_thread + end ensure conflicting_exclusive_threads.each(&:kill) + no_purpose_thread.kill end end end -- cgit v1.2.3 From e9020ac4310d1b190619769a8a621935d4efc812 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Tue, 21 Jul 2015 11:48:21 +0930 Subject: Handle thread death during lock acquisition Specifically, clean up if the thread is killed while it's blocked awaiting the lock... if we get killed on some other arbitrary line, the result remains quite undefined. --- .../lib/active_support/concurrency/share_lock.rb | 10 ++++++---- activesupport/test/share_lock_test.rb | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+), 4 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 39ae9bfb79..ca48164c54 100644 --- a/activesupport/lib/active_support/concurrency/share_lock.rb +++ b/activesupport/lib/active_support/concurrency/share_lock.rb @@ -54,10 +54,12 @@ module ActiveSupport loose_shares = @sharing.delete(Thread.current) @waiting[Thread.current] = compatible if loose_shares - @cv.wait_while { busy?(purpose) } - - @waiting.delete Thread.current - @sharing[Thread.current] = loose_shares if loose_shares + begin + @cv.wait_while { busy?(purpose) } + ensure + @waiting.delete Thread.current + @sharing[Thread.current] = loose_shares if loose_shares + end end @exclusive_thread = Thread.current end diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index efd840be79..4c0d23784e 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -81,6 +81,27 @@ class ShareLockTest < ActiveSupport::TestCase end end + def test_killed_thread_loses_lock + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + thread = Thread.new do + @lock.sharing do + @lock.exclusive {} + end + end + + assert_threads_stuck thread + thread.kill + + sharing_thread_release_latch.count_down + + thread = Thread.new do + @lock.exclusive {} + end + + assert_threads_not_stuck thread + end + end + def test_exclusive_conflicting_purpose [true, false].each do |use_upgrading| with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| -- cgit v1.2.3 From 5d6770754e1a4364ccfd476412d9abb538baf936 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Wed, 22 Jul 2015 22:59:01 +0200 Subject: Small tweaks to mainly lock-ordering tests. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * only test the upgrade path, * add test to verify non upgrades can’t preempt, * add reentrancy assertion. --- activesupport/test/share_lock_test.rb | 79 +++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 21 deletions(-) (limited to 'activesupport') diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index 4c0d23784e..ad41db608b 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -7,6 +7,14 @@ class ShareLockTest < ActiveSupport::TestCase @lock = ActiveSupport::Concurrency::ShareLock.new end + def test_reentrancy + thread = Thread.new do + @lock.sharing { @lock.sharing {} } + @lock.exclusive { @lock.exclusive {} } + end + assert_threads_not_stuck thread + end + def test_sharing_doesnt_block with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_latch| assert_threads_not_stuck(Thread.new {@lock.sharing {} }) @@ -169,38 +177,67 @@ class ShareLockTest < ActiveSupport::TestCase end def test_exclusive_ordering - [true, false].each do |use_upgrading| - scratch_pad = [] - scratch_pad_mutex = Mutex.new + scratch_pad = [] + scratch_pad_mutex = Mutex.new - load_params = [:load, [:load]] - unload_params = [:unload, [:unload, :load]] + load_params = [:load, [:load]] + unload_params = [:unload, [:unload, :load]] - [load_params, load_params, unload_params, unload_params].permutation do |thread_params| - with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| - threads = thread_params.map do |purpose, compatible| - Thread.new do - @lock.send(use_upgrading ? :sharing : :tap) do - @lock.exclusive(purpose: purpose, compatible: compatible) do - scratch_pad_mutex.synchronize { scratch_pad << purpose } - end + [load_params, load_params, unload_params, unload_params].permutation do |thread_params| + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + threads = thread_params.map do |purpose, compatible| + Thread.new do + @lock.sharing do + @lock.exclusive(purpose: purpose, compatible: compatible) do + scratch_pad_mutex.synchronize { scratch_pad << purpose } end end end + end - sleep(0.01) - scratch_pad_mutex.synchronize { assert_empty scratch_pad } + sleep(0.01) + scratch_pad_mutex.synchronize { assert_empty scratch_pad } - sharing_thread_release_latch.count_down + sharing_thread_release_latch.count_down - assert_threads_not_stuck threads - scratch_pad_mutex.synchronize do - if use_upgrading - assert_equal [:load, :load, :unload, :unload], scratch_pad + assert_threads_not_stuck threads + scratch_pad_mutex.synchronize do + assert_equal [:load, :load, :unload, :unload], scratch_pad + scratch_pad.clear + end + end + end + end + + def test_in_shared_section_incompatible_non_upgrading_threads_cannot_preempt_upgrading_threads + scratch_pad = [] + scratch_pad_mutex = Mutex.new + + upgrading_load_params = [:load, [:load], true] + non_upgrading_unload_params = [:unload, [:load, :unload], false] + + [upgrading_load_params, non_upgrading_unload_params].permutation do |thread_params| + with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| + threads = thread_params.map do |purpose, compatible, use_upgrading| + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + @lock.exclusive(purpose: purpose, compatible: compatible) do + scratch_pad_mutex.synchronize { scratch_pad << purpose } + end end - scratch_pad.clear end end + + assert_threads_stuck threads + scratch_pad_mutex.synchronize { assert_empty scratch_pad } + + sharing_thread_release_latch.count_down + + assert_threads_not_stuck threads + scratch_pad_mutex.synchronize do + assert_equal [:load, :unload], scratch_pad + scratch_pad.clear + end end end end -- cgit v1.2.3