From 23a7714343b372234972ef0dacf774d07fe65ced Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Fri, 24 Nov 2023 13:18:00 +0100 Subject: Refactor and fix the GVL instrumentation API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This entirely changes how it is tested. Rather than to use counters we now record the timeline of events with associated threads which makes it much easier to assert that certains events are only preceded by a specific event, and makes it much easier to debug unexpected timelines. Co-Authored-By: Étienne Barrié Co-Authored-By: JP Camara Co-Authored-By: John Hawthorn --- test/-ext-/thread/test_instrumentation_api.rb | 209 ++++++++++++++++++++------ 1 file changed, 167 insertions(+), 42 deletions(-) (limited to 'test/-ext-') diff --git a/test/-ext-/thread/test_instrumentation_api.rb b/test/-ext-/thread/test_instrumentation_api.rb index 208d11de85..e2df02f81a 100644 --- a/test/-ext-/thread/test_instrumentation_api.rb +++ b/test/-ext-/thread/test_instrumentation_api.rb @@ -7,75 +7,200 @@ class TestThreadInstrumentation < Test::Unit::TestCase require '-test-/thread/instrumentation' - Thread.list.each do |thread| - if thread != Thread.current - thread.kill - thread.join rescue nil - end - end - assert_equal [Thread.current], Thread.list - - Bug::ThreadInstrumentation.reset_counters - Bug::ThreadInstrumentation::register_callback + cleanup_threads end def teardown return if /mswin|mingw|bccwin/ =~ RUBY_PLATFORM - Bug::ThreadInstrumentation::unregister_callback - Bug::ThreadInstrumentation.last_spawned_thread = nil + Bug::ThreadInstrumentation.unregister_callback + cleanup_threads end THREADS_COUNT = 3 - def test_thread_instrumentation - threads = threaded_cpu_work - assert_equal [false] * THREADS_COUNT, threads.map(&:status) - counters = Bug::ThreadInstrumentation.counters - assert_join_counters(counters) - assert_global_join_counters(counters) + def test_single_thread_timeline + thread = nil + full_timeline = record do + thread = Thread.new { 1 + 1 } + thread.join + end + assert_equal %i(started ready resumed suspended exited), timeline_for(thread, full_timeline) + ensure + thread&.kill + end + + def test_muti_thread_timeline + threads = nil + full_timeline = record do + threads = threaded_cpu_work + fib(20) + assert_equal [false] * THREADS_COUNT, threads.map(&:status) + end + + + threads.each do |thread| + timeline = timeline_for(thread, full_timeline) + assert_consistent_timeline(timeline) + end + + timeline = timeline_for(Thread.current, full_timeline) + assert_consistent_timeline(timeline) + ensure + threads&.each(&:kill) + end + + def test_join_suspends # Bug #18900 + thread = other_thread = nil + full_timeline = record do + other_thread = Thread.new { sleep 0.3 } + thread = Thread.new { other_thread.join } + thread.join + end + + timeline = timeline_for(thread, full_timeline) + assert_consistent_timeline(timeline) + assert_equal %i(started ready resumed suspended ready resumed suspended exited), timeline + ensure + other_thread&.kill + thread&.kill + end + + def test_io_release_gvl + r, w = IO.pipe + thread = nil + full_timeline = record do + thread = Thread.new do + w.write("Hello\n") + end + thread.join + end + + timeline = timeline_for(thread, full_timeline) + assert_consistent_timeline(timeline) + assert_equal %i(started ready resumed suspended ready resumed suspended exited), timeline + ensure + r&.close + w&.close + end + + def test_queue_releases_gvl + queue1 = Queue.new + queue2 = Queue.new + + thread = nil + + full_timeline = record do + thread = Thread.new do + queue1 << true + queue2.pop + end + + queue1.pop + queue2 << true + thread.join + end + + timeline = timeline_for(thread, full_timeline) + assert_consistent_timeline(timeline) + assert_equal %i(started ready resumed suspended ready resumed suspended exited), timeline end - def test_join_counters # Bug #18900 - thr = Thread.new { fib(30) } - Bug::ThreadInstrumentation.reset_counters - thr.join - assert_join_counters(Bug::ThreadInstrumentation.local_counters) + def test_thread_blocked_forever + mutex = Mutex.new + mutex.lock + thread = nil + + full_timeline = record do + thread = Thread.new do + mutex.lock + end + 10.times { Thread.pass } + sleep 0.1 + end + + mutex.unlock + thread.join + + timeline = timeline_for(thread, full_timeline) + assert_consistent_timeline(timeline) + assert_equal %i(started ready resumed), timeline end def test_thread_instrumentation_fork_safe skip "No fork()" unless Process.respond_to?(:fork) - thread_statuses = counters = nil + thread_statuses = full_timeline = nil IO.popen("-") do |read_pipe| if read_pipe thread_statuses = Marshal.load(read_pipe) - counters = Marshal.load(read_pipe) + full_timeline = Marshal.load(read_pipe) else - Bug::ThreadInstrumentation.reset_counters threads = threaded_cpu_work Marshal.dump(threads.map(&:status), STDOUT) - Marshal.dump(Bug::ThreadInstrumentation.counters, STDOUT) + full_timeline = Bug::ThreadInstrumentation.unregister_callback.map { |t, e| [t.to_s, e ] } + Marshal.dump(full_timeline, STDOUT) end end assert_predicate $?, :success? assert_equal [false] * THREADS_COUNT, thread_statuses - assert_join_counters(counters) - assert_global_join_counters(counters) + thread_names = full_timeline.map(&:first).uniq + thread_names.each do |thread_name| + assert_consistent_timeline(timeline_for(thread_name, full_timeline)) + end end def test_thread_instrumentation_unregister - Bug::ThreadInstrumentation::unregister_callback assert Bug::ThreadInstrumentation::register_and_unregister_callbacks end - def test_thread_instrumentation_event_data - assert_nil Bug::ThreadInstrumentation.last_spawned_thread - thr = Thread.new{ }.join - assert_same thr, Bug::ThreadInstrumentation.last_spawned_thread + private + + def record + Bug::ThreadInstrumentation.register_callback + yield + ensure + timeline = Bug::ThreadInstrumentation.unregister_callback + if $! + raise + else + return timeline + end + end + + def assert_consistent_timeline(events) + refute_predicate events, :empty? + + previous_event = nil + events.each do |event| + refute_equal :exited, previous_event, "`exited` must be the final event: #{events.inspect}" + case event + when :started + assert_nil previous_event, "`started` must be the first event: #{events.inspect}" + when :ready + unless previous_event.nil? + assert %i(started suspended).include?(previous_event), "`ready` must be preceded by `started` or `suspended`: #{events.inspect}" + end + when :resumed + unless previous_event.nil? + assert_equal :ready, previous_event, "`resumed` must be preceded by `ready`: #{events.inspect}" + end + when :suspended + unless previous_event.nil? + assert_equal :resumed, previous_event, "`suspended` must be preceded by `resumed`: #{events.inspect}" + end + when :exited + unless previous_event.nil? + assert %i(resumed suspended).include?(previous_event), "`exited` must be preceded by `resumed` or `suspended`: #{events.inspect}" + end + end + previous_event = event + end end - private + def timeline_for(thread, timeline) + timeline.select { |t, _| t == thread }.map(&:last) + end def fib(n = 20) return n if n <= 1 @@ -86,13 +211,13 @@ class TestThreadInstrumentation < Test::Unit::TestCase THREADS_COUNT.times.map { Thread.new { fib(size) } }.each(&:join) end - def assert_join_counters(counters) - counters.each_with_index do |c, i| - assert_operator c, :>, 0, "Call counters[#{i}]: #{counters.inspect}" + def cleanup_threads + Thread.list.each do |thread| + if thread != Thread.current + thread.kill + thread.join rescue nil + end end - end - - def assert_global_join_counters(counters) - assert_equal THREADS_COUNT, counters.first + assert_equal [Thread.current], Thread.list end end -- cgit v1.2.3