Rebuild --watch event loop to simplify; fix concurrency issues (#40)

* Rebuild `--watch` event loop to simplify, fix concurrency issues

Before, the watcher event loop relied on multiple threads and raised an
exception to interrupt the main thread whenever a file system change
event happened. This was messy and resulted in zombie processes and
other weird behavior when a file system change was detected in the
middle of running tests.

This commit refactors the implementation to use a new `EventQueue`
class. It leverages the buffering inherent in stdin, plus uses a
thread-safe queue for placing file system change events, and then "pops"
the next of these in each iteration of the event loop.

To avoid starting a separate thread for reading key presses, the "pop"
operation takes turns polling stdin (peeking at its buffer to see if a
key press happened) and blocking for short period waiting for file
system events.

The result is cleaner, easier to test, and not prone to the zombie
process bug.

* Require io/wait for older Ruby versions

* Add workaround for Ruby 3.1

lib/mighty_test/console.rb

@@ -1,4 +1,5 @@
 require "io/console"
+require "io/wait"
 
 module MightyTest
   class Console
@@ -15,11 +16,14 @@ def clear
       true
     end
 
-    def wait_for_keypress
-      with_raw_input do
-        sleep if stdin.eof?
-        stdin.getc
-      end
+    def with_raw_input(&)
+      return yield unless stdin.respond_to?(:raw) && tty?
+
+      stdin.raw(intr: true, &)
+    end
+
+    def read_keypress_nonblock
+      stdin.getc if stdin.wait_readable(0)
     end
 
     def play_sound(name, wait: false)
@@ -55,11 +59,5 @@ def play_sound(name, wait: false)
     def tty?
       $stdout.respond_to?(:tty?) && $stdout.tty?
     end
-
-    def with_raw_input(&)
-      return yield unless stdin.respond_to?(:raw) && tty?
-
-      stdin.raw(intr: true, &)
-    end
   end
 end

lib/mighty_test/watcher.rb

@@ -1,58 +1,41 @@
+require_relative "watcher/event_queue"
+
 module MightyTest
   class Watcher
-    class ListenerTriggered < StandardError
-      attr_reader :paths
-
-      def initialize(paths)
-        @paths = paths
-        super()
-      end
-    end
-
     WATCHING_FOR_CHANGES = 'Watching for changes to source and test files. Press "h" for help or "q" to quit.'.freeze
 
-    def initialize(console: Console.new, extra_args: [], file_system: FileSystem.new, system_proc: method(:system))
+    def initialize(console: Console.new, extra_args: [], event_queue: nil, file_system: nil, system_proc: nil)
       @console = console
       @extra_args = extra_args
-      @file_system = file_system
-      @system_proc = system_proc
+      @file_system = file_system || FileSystem.new
+      @system_proc = system_proc || method(:system)
+      @event_queue = event_queue || EventQueue.new(console: @console, file_system: @file_system)
     end
 
-    def run(iterations: :indefinitely) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength
-      started = false
-      @foreground_thread = Thread.current
-
-      loop_for(iterations) do
-        start_file_system_listener && puts(WATCHING_FOR_CHANGES) unless started
-        started = true
-
-        case console.wait_for_keypress
-        when "\r", "\n"
-          run_all_tests
-        when "a"
-          run_all_tests(flags: ["--all"])
-        when "d"
-          run_matching_test_files_from_git_diff
-        when "h"
-          show_help
-        when "q"
-          file_system_listener.stop
-          break
+    def run
+      event_queue.start
+      puts WATCHING_FOR_CHANGES
+
+      loop do
+        case event_queue.pop
+        in [:file_system_changed, [_, *] => paths] then run_matching_test_files(paths)
+        in [:keypress, "\r" | "\n"] then run_all_tests
+        in [:keypress, "a"] then run_all_tests(flags: ["--all"])
+        in [:keypress, "d"] then run_matching_test_files_from_git_diff
+        in [:keypress, "h"] then show_help
+        in [:keypress, "q"] then break
+        else
+          nil
         end
-      rescue ListenerTriggered => e
-        run_matching_test_files(e.paths)
-        file_system_listener.start if file_system_listener.paused?
-      rescue Interrupt
-        file_system_listener&.stop
-        raise
       end
     ensure
+      event_queue.stop
       puts "\nExiting."
     end
 
     private
 
-    attr_reader :console, :extra_args, :file_system, :file_system_listener, :system_proc, :foreground_thread
+    attr_reader :console, :extra_args, :file_system, :event_queue, :system_proc
 
     def show_help
       console.clear
@@ -109,26 +92,7 @@ def mt(*test_paths, flags: [])
       $stdout.flush
     rescue Interrupt
       # Pressing ctrl-c kills the fs_event background process, so we have to manually restart it.
-      # Do this in a separate thread to work around odd behavior on Ruby 3.4.
-      Thread.new { restart_file_system_listener }
-    end
-
-    def start_file_system_listener
-      file_system_listener.stop if file_system_listener && !file_system_listener.stopped?
-
-      @file_system_listener = file_system.listen do |modified, added, _removed|
-        paths = [*modified, *added].uniq
-        next if paths.empty?
-
-        # Pause listener so that subsequent changes are queued up while we are running the tests
-        file_system_listener.pause unless file_system_listener.stopped?
-        foreground_thread.raise ListenerTriggered.new(paths)
-      end
-    end
-    alias restart_file_system_listener start_file_system_listener
-
-    def loop_for(iterations, &)
-      iterations == :indefinitely ? loop(&) : iterations.times(&)
+      event_queue.restart
     end
   end
 end

lib/mighty_test/watcher/event_queue.rb

@@ -0,0 +1,78 @@
+require "io/console"
+
+module MightyTest
+  class Watcher
+    class EventQueue
+      def initialize(console: Console.new, file_system: FileSystem.new)
+        @console = console
+        @file_system = file_system
+        @file_system_queue = Thread::Queue.new
+      end
+
+      def pop
+        console.with_raw_input do
+          until stopped?
+            if (key = console.read_keypress_nonblock)
+              return [:keypress, key]
+            end
+            if (paths = pop_files_changed)
+              return [:file_system_changed, paths]
+            end
+          end
+        end
+      end
+
+      def start
+        raise "Already started" unless stopped?
+
+        @file_system_listener = file_system.listen do |modified, added, _removed|
+          paths = [*modified, *added].uniq
+          file_system_queue.push(paths) unless paths.empty?
+        end
+        true
+      end
+
+      def restart
+        stop
+        start
+      end
+
+      def stop
+        file_system_listener&.stop
+        @file_system_listener = nil
+      end
+
+      def stopped?
+        !file_system_listener
+      end
+
+      private
+
+      attr_reader :console, :file_system, :file_system_listener, :file_system_queue
+
+      def pop_files_changed
+        paths = try_file_system_pop(timeout: 0.2)
+        return if paths.nil?
+
+        paths += file_system_queue.pop until file_system_queue.empty?
+        paths.uniq
+      end
+
+      if RUBY_VERSION.start_with?("3.1.")
+        # TODO: Remove once we drop support for Ruby 3.1
+        require "timeout"
+        def try_file_system_pop(timeout:)
+          Timeout.timeout(timeout) do
+            file_system_queue.pop
+          end
+        rescue Timeout::Error
+          nil
+        end
+      else
+        def try_file_system_pop(timeout:)
+          file_system_queue.pop(timeout:)
+        end
+      end
+    end
+  end
+end

test/mighty_test/console_test.rb

@@ -20,10 +20,20 @@ def test_clear_clears_the_screen_and_returns_true_and_if_tty
       assert_equal "clear!", stdout
     end
 
-    def test_wait_for_keypress_returns_the_next_character_on_stdin
-      console = Console.new(stdin: StringIO.new("hi"))
+    def test_read_keypress_nonblock_returns_the_next_character_on_stdin
+      stdin = StringIO.new("hi")
+      stdin.define_singleton_method(:wait_readable) { |_timeout| true }
+      console = Console.new(stdin:)
 
-      assert_equal "h", console.wait_for_keypress
+      assert_equal "h", console.read_keypress_nonblock
+    end
+
+    def test_read_keypress_nonblock_returns_nil_if_nothing_is_in_buffer
+      stdin = StringIO.new
+      stdin.define_singleton_method(:wait_readable) { |_timeout| false }
+      console = Console.new(stdin:)
+
+      assert_nil console.read_keypress_nonblock
     end
 
     def test_play_sound_returns_false_if_not_tty