Refactor and fix the GVL instrumentation API

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é <etienne.barrie@gmail.com>
Co-Authored-By: JP Camara <jp@jpcamara.com>
Co-Authored-By: John Hawthorn <john@hawthorn.email>

1 files changed, 167 insertions, 42 deletions

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