Fix TinyServer races.jk/tinyserver-race

1. Wait for the start callback rather than the do block, as it happens
   later. This prevents us from getting into many conditions where the
   start returns before the server is fully initialized, where if stop()
   is called too soon after start returns, it would cause exceptions.
2. Wait for the thread to finish completely
3. Use Thread.current.abort_on_exception` to show the actual listen errors,
   so that problems like this can be diagnosed.
4. Use :each to start and stop TinyServer so that race conditions like this
   are exposed earlier and more often (this will let us rid ourselves of them
   more quickly).
5. Use WEBrick::HTTPServer directly, which gets rid of the need for the
   complicated trap workaround (rack is the one that traps).

1 files changed, 53 insertions, 41 deletions

diff --git a/spec/tiny_server.rb b/spec/tiny_server.rb
index 7a3ca99c9f..83c5bf4a42 100644
--- a/spec/tiny_server.rb
+++ b/spec/tiny_server.rb
@@ -27,63 +27,75 @@ require "chef/config"
 
 module TinyServer
 
-  class Server < Rack::Server
-
-    attr_writer :app
-
-    def self.setup(options = nil, &block)
-      tiny_app = new(options)
-      app_code = Rack::Builder.new(&block).to_app
-      tiny_app.app = app_code
-      tiny_app
-    end
-
-    def shutdown
-      server.shutdown
-    end
-  end
-
   class Manager
 
     # 5 == debug, 3 == warning
     LOGGER = WEBrick::Log.new(STDOUT, 3)
     DEFAULT_OPTIONS = {
-      :server => "webrick",
-      :Port => 9000,
-      :Host => "localhost",
-      :environment => :none,
-      :Logger => LOGGER,
-      :AccessLog => [] # Remove this option to enable the access log when debugging.
+      Port: 9000,
+      Host: "localhost",
+      Logger: LOGGER,
+      # SSLEnable: options[:ssl],
+      # SSLCertName: [ [ 'CN', WEBrick::Utils::getservername ] ],
+      AccessLog: [], # Remove this option to enable the access log when debugging.
     }
 
-    def initialize(options = nil)
-      @options = options ? DEFAULT_OPTIONS.merge(options) : DEFAULT_OPTIONS
+    def initialize(**options)
+      @options = DEFAULT_OPTIONS.merge(options)
       @creator = caller.first
     end
 
-    def start
-      started = Queue.new
+    attr_reader :options
+    attr_reader :creator
+    attr_reader :server
+
+    def start(timeout = 5)
+      raise "Server already started!" if server
+
+      # Create the server (but don't start yet)
+      start_queue = Queue.new
+      @server = create_server(StartCallback: proc { start_queue << true })
+
       @server_thread = Thread.new do
-        @server = Server.setup(@options) do
-          run API.instance
-        end
-        @old_handler = trap(:INT, "EXIT")
-        @server.start do
-          started << true
-        end
+        # Ensure any exceptions will cause the main rspec thread to fail too
+        Thread.current.abort_on_exception = true
+        server.start
+      end
+
+      # Wait for the StartCallback to tell us we've started
+      Timeout.timeout(timeout) do
+        start_queue.pop
       end
-      started.pop
-      trap(:INT, @old_handler)
     end
 
-    def stop
-      # yes, this is terrible.
-      @server.shutdown
-      @server_thread.kill
-      @server_thread.join
-      @server_thread = nil
+    def stop(timeout = 5)
+      if server
+        server.shutdown
+        @server = nil
+      end
+
+      if server_thread
+        begin
+          # Wait for a normal shutdown
+          server_thread.join(timeout)
+        rescue
+          # If it wouldn't shut down normally, kill it.
+          server_thread.kill
+          server_thread.join(timeout)
+        end
+        @server_thread = nil
+      end
     end
 
+    private
+
+    attr_reader :server_thread
+
+    def create_server(**extra_options)
+      server = WEBrick::HTTPServer.new(**options, **extra_options)
+      server.mount("/", Rack::Handler::WEBrick, API.instance)
+      server
+    end
   end
 
   class API