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).

9 files changed, 96 insertions, 74 deletions

diff --git a/spec/functional/http/simple_spec.rb b/spec/functional/http/simple_spec.rb
index aeb7be7d86..421045693a 100644
--- a/spec/functional/http/simple_spec.rb
+++ b/spec/functional/http/simple_spec.rb
@@ -26,11 +26,11 @@ describe Chef::HTTP::Simple do
   let(:http_client) { described_class.new(source) }
   let(:http_client_disable_gzip) { described_class.new(source, { :disable_gzip => true } ) }
 
-  before(:all) do
+  before(:each) do
     start_tiny_server
   end
 
-  after(:all) do
+  after(:each) do
     stop_tiny_server
   end
 
diff --git a/spec/functional/knife/cookbook_delete_spec.rb b/spec/functional/knife/cookbook_delete_spec.rb
index a43e3a36c4..99f3309752 100644
--- a/spec/functional/knife/cookbook_delete_spec.rb
+++ b/spec/functional/knife/cookbook_delete_spec.rb
@@ -20,11 +20,15 @@ require "spec_helper"
 require "tiny_server"
 
 describe Chef::Knife::CookbookDelete do
-  before(:all) do
+  before(:each) do
     @server = TinyServer::Manager.new
     @server.start
   end
 
+  after(:each) do
+    @server.stop
+  end
+
   before(:each) do
     @knife = Chef::Knife::CookbookDelete.new
     @api = TinyServer::API.instance
@@ -35,10 +39,6 @@ describe Chef::Knife::CookbookDelete do
     Chef::Config[:chef_server_url] = "http://localhost:9000"
   end
 
-  after(:all) do
-    @server.stop
-  end
-
   context "when the cookbook doesn't exist" do
     let(:log_output) { StringIO.new }
 
diff --git a/spec/functional/knife/exec_spec.rb b/spec/functional/knife/exec_spec.rb
index 74d006a8b8..ac8f617a90 100644
--- a/spec/functional/knife/exec_spec.rb
+++ b/spec/functional/knife/exec_spec.rb
@@ -20,11 +20,15 @@ require "spec_helper"
 require "tiny_server"
 
 describe Chef::Knife::Exec do
-  before(:all) do
+  before(:each) do
     @server = TinyServer::Manager.new #(:debug => true)
     @server.start
   end
 
+  after(:each) do
+    @server.stop
+  end
+
   before(:each) do
     @knife = Chef::Knife::Exec.new
     @api = TinyServer::API.instance
@@ -37,10 +41,6 @@ describe Chef::Knife::Exec do
     $output = StringIO.new
   end
 
-  after(:all) do
-    @server.stop
-  end
-
   it "executes a script in the context of the chef-shell main context" do
     @node = Chef::Node.new
     @node.name("ohai-world")
diff --git a/spec/functional/knife/ssh_spec.rb b/spec/functional/knife/ssh_spec.rb
index 0f51b765af..065b646ac6 100644
--- a/spec/functional/knife/ssh_spec.rb
+++ b/spec/functional/knife/ssh_spec.rb
@@ -21,13 +21,13 @@ require "tiny_server"
 
 describe Chef::Knife::Ssh do
 
-  before(:all) do
+  before(:each) do
     Chef::Knife::Ssh.load_deps
     @server = TinyServer::Manager.new
     @server.start
   end
 
-  after(:all) do
+  after(:each) do
     @server.stop
   end
 
diff --git a/spec/functional/resource/remote_file_spec.rb b/spec/functional/resource/remote_file_spec.rb
index b394bd0240..1f92a567f3 100644
--- a/spec/functional/resource/remote_file_spec.rb
+++ b/spec/functional/resource/remote_file_spec.rb
@@ -55,11 +55,11 @@ describe Chef::Resource::RemoteFile do
   let(:default_mode) { (0666 & ~File.umask).to_s(8) }
 
   context "when fetching files over HTTP" do
-    before(:all) do
+    before(:each) do
       start_tiny_server
     end
 
-    after(:all) do
+    after(:each) do
       stop_tiny_server
     end
 
@@ -97,7 +97,7 @@ describe Chef::Resource::RemoteFile do
 
   context "when fetching files over HTTPS" do
 
-    before(:all) do
+    before(:each) do
       cert_text = File.read(File.expand_path("ssl/chef-rspec.cert", CHEF_SPEC_DATA))
       cert = OpenSSL::X509::Certificate.new(cert_text)
       key_text = File.read(File.expand_path("ssl/chef-rspec.key", CHEF_SPEC_DATA))
@@ -111,7 +111,7 @@ describe Chef::Resource::RemoteFile do
       start_tiny_server(server_opts)
     end
 
-    after(:all) do
+    after(:each) do
       stop_tiny_server
     end
 
@@ -124,11 +124,11 @@ describe Chef::Resource::RemoteFile do
   end
 
   context "when dealing with content length checking" do
-    before(:all) do
+    before(:each) do
       start_tiny_server
     end
 
-    after(:all) do
+    after(:each) do
       stop_tiny_server
     end
 
@@ -232,7 +232,16 @@ describe Chef::Resource::RemoteFile do
       end
 
       it "should not create the file" do
-        expect { resource.run_action(:create) }.to raise_error
+        # This can legitimately raise either Errno::EADDRNOTAVAIL or Errno::ECONNREFUSED
+        # in different Ruby versions.
+        old_value = RSpec::Expectations.configuration.on_potential_false_positives
+        RSpec::Expectations.configuration.on_potential_false_positives = :nothing
+        begin
+          expect { resource.run_action(:create) }.to raise_error
+        ensure
+          RSpec::Expectations.configuration.on_potential_false_positives = old_value
+        end
+
         expect(File).not_to exist(path)
       end
     end
diff --git a/spec/functional/rest_spec.rb b/spec/functional/rest_spec.rb
index adafc18e5a..14e76087c4 100644
--- a/spec/functional/rest_spec.rb
+++ b/spec/functional/rest_spec.rb
@@ -83,11 +83,11 @@ describe Chef::REST do
     Chef::Config[:treat_deprecation_warnings_as_errors] = false
   end
 
-  before(:all) do
+  before(:each) do
     start_tiny_server
   end
 
-  after(:all) do
+  after(:each) do
     stop_tiny_server
   end
 
diff --git a/spec/functional/tiny_server_spec.rb b/spec/functional/tiny_server_spec.rb
index 2a025a2ecd..1ec56bd490 100644
--- a/spec/functional/tiny_server_spec.rb
+++ b/spec/functional/tiny_server_spec.rb
@@ -65,14 +65,15 @@ end
 
 describe TinyServer::Manager do
   it "runs the server" do
-    @server = TinyServer::Manager.new
-    @server.start
+    server = TinyServer::Manager.new
+    server.start
+    begin
+      TinyServer::API.instance.get("/index", 200, "[\"hello\"]")
 
-    TinyServer::API.instance.get("/index", 200, "[\"hello\"]")
-
-    rest = Chef::HTTP.new("http://localhost:9000")
-    expect(rest.get("index")).to eq("[\"hello\"]")
-
-    @server.stop
+      rest = Chef::HTTP.new("http://localhost:9000")
+      expect(rest.get("index")).to eq("[\"hello\"]")
+    ensure
+      server.stop
+    end
   end
 end
diff --git a/spec/integration/client/client_spec.rb b/spec/integration/client/client_spec.rb
index 4d406ea495..da3a2b98e4 100644
--- a/spec/integration/client/client_spec.rb
+++ b/spec/integration/client/client_spec.rb
@@ -476,11 +476,11 @@ end
 
   # Fails on appveyor, but works locally on windows and on windows hosts in Ci.
   context "when using recipe-url", :skip_appveyor do
-    before(:all) do
+    before(:each) do
       start_tiny_server
     end
 
-    after(:all) do
+    after(:each) do
       stop_tiny_server
     end
 
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