Don't crash on client disconnect (Server and Application)

CHANGELOG.md

@@ -1,3 +1,7 @@
+## Next Release
+
+* Fixed crashes when a client disconnects mid-handshake (e.g. on connect timeout). Previously, `Errno::EPIPE` raised in `Spring::Server#serve` or `Spring::Application#serve` would propagate up through the accept loop and kill the process, leaving a stale socket that broke every subsequent client. Both crash sites are now rescued, including writes that happen inside the `rescue Exception` handler in `Application#serve` while reporting an earlier failure to the gone client.
+
 ## 4.4.0
 
 * Revert the removal of UTF-8 force encoding in JSON loading.

lib/spring/application.rb

@@ -185,7 +185,7 @@ def serve(client)
           client.puts(0) # preload success
         rescue Exception
           log "preload failed"
-          client.puts(1) # preload failure
+          ignore_client_disconnect { client.puts(1) } # preload failure
           raise
         end
       end
@@ -246,15 +246,19 @@ def serve(client)
 
       wait pid, streams, client
     rescue Exception => e
-      log "exception: #{e}"
+      if e.is_a?(Errno::EPIPE)
+        log "client disconnected (#{e.message}), ignoring command"
+      else
+        log "exception: #{e}"
+      end
       manager.puts unless pid
 
       if streams && !e.is_a?(SystemExit)
-        print_exception(stderr, e)
-        streams.each(&:close)
+        ignore_client_disconnect { print_exception(stderr, e) }
+        streams.each { |stream| ignore_client_disconnect { stream.close } }
       end
 
-      client.puts(1) if pid
+      ignore_client_disconnect { client.puts(1) if pid }
       client.close
     ensure
       # Redirect STDOUT and STDERR to prevent from keeping the original FDs
@@ -411,6 +415,15 @@ def wait(pid, streams, client)
 
     private
 
+    # Tolerate Errno::EPIPE on writes to the client socket. Once the client
+    # disconnects, every subsequent write to it raises — that's expected
+    # during `serve`'s status reporting; we'd be talking to a process
+    # that's gone.
+    def ignore_client_disconnect
+      yield
+    rescue Errno::EPIPE
+    end
+
     def active_record_configured?
       defined?(ActiveRecord::Base) && ActiveRecord::Base.configurations.any?
     end

lib/spring/server.rb

@@ -74,6 +74,8 @@ def serve(client)
       end
     rescue SocketError => e
       raise e unless client.eof?
+    rescue Errno::EPIPE => e
+      log "client disconnected with error #{e.message}, ignoring command"
     ensure
       redirect_output
     end

test/unit/application_test.rb

@@ -0,0 +1,110 @@
+require_relative "../helper"
+require "spring/env"
+require "spring/application"
+require "timeout"
+
+# Regression tests for https://github.com/rails/spring/issues/724:
+# disconnected client raises Errno::EPIPE in #serve, which used to crash
+# the application process. Each test exercises one of the EPIPE call
+# sites in `Spring::Application#serve` with a `UNIXSocket.pair` whose
+# client side has been closed.
+class ApplicationTest < ActiveSupport::TestCase
+  def setup
+    @log_file = File.open(File::NULL, "a")
+    @spring_env = Spring::Env.new(log_file: @log_file)
+    @manager_read, @manager_write = UNIXSocket.pair
+    @app = Spring::Application.new(@manager_write, {}, @spring_env)
+    @server_sock = build_disconnected_application_client
+  end
+
+  def teardown
+    @server_sock&.close
+    @manager_read&.close
+    @manager_write&.close
+    @log_file&.close
+  end
+
+  test "#serve does not raise when the client disconnects before the cached-preload-success write" do
+    @app.define_singleton_method(:preloaded?) { true }
+
+    with_saved_stdio { @app.serve(@server_sock) }
+
+    assert_manager_handshake_complete
+  end
+
+  test "#serve does not raise when the client disconnects before the fresh-preload-success write" do
+    @app.define_singleton_method(:preload) {}
+
+    with_saved_stdio { @app.serve(@server_sock) }
+
+    assert_manager_handshake_complete
+  end
+
+  test "#serve does not raise when the client disconnects during preload-failure recovery" do
+    @app.define_singleton_method(:preload) { raise RuntimeError, "simulated preload failure" }
+
+    with_saved_stdio { @app.serve(@server_sock) }
+
+    assert_manager_handshake_complete
+  end
+
+  private
+
+  # Simulates a client that handed off its STDOUT/STDERR/STDIN to the
+  # application and then died. The 3 stream FDs sent to the application:
+  #
+  #   - STDOUT, STDERR: write-ends of pipes whose read ends are closed, so
+  #     writes (e.g. via `print_exception`) raise Errno::EPIPE.
+  #   - STDIN: read-end of a pipe whose write end is closed, so reads return
+  #     EOF (matches a dead client's STDIN).
+  #
+  # The UNIXSocket itself is also closed on the client side, so writes to
+  # `client` (e.g. `client.puts(0)`) raise Errno::EPIPE too.
+  def build_disconnected_application_client
+    server_sock, client_sock = UNIXSocket.pair
+
+    out_r, out_w = IO.pipe
+    err_r, err_w = IO.pipe
+    in_r,  in_w  = IO.pipe
+    [[out_r, out_w], [err_r, err_w]].each do |r, w|
+      client_sock.send_io(w)
+      r.close
+      w.close
+    end
+    client_sock.send_io(in_r)
+    in_r.close
+    in_w.close
+
+    client_sock.close
+    server_sock
+  end
+
+  # Application#serve reopens STDOUT, STDERR, and STDIN to the streams it
+  # received from the client (and to its log_file in `reset_streams`).
+  # Save and restore the test runner's streams around the call.
+  def with_saved_stdio
+    saved_out = STDOUT.dup
+    saved_err = STDERR.dup
+    saved_in  = STDIN.dup
+    yield
+  ensure
+    STDOUT.reopen(saved_out) if saved_out
+    STDERR.reopen(saved_err) if saved_err
+    STDIN.reopen(saved_in)   if saved_in
+    [saved_out, saved_err, saved_in].compact.each { |io| io.close rescue nil }
+  end
+
+  # `serve` writes two newlines to the manager: an early "got client" ack
+  # and a no-pid response from the rescue handler. Without the second,
+  # `ApplicationManager#run` blocks forever on `child.gets.to_i` and the
+  # server's single-threaded accept loop deadlocks. Timeout + flunk fails
+  # loudly instead of stalling the suite.
+  def assert_manager_handshake_complete
+    acks = Timeout.timeout(2) { [@manager_read.gets, @manager_read.gets] }
+    assert_equal ["\n", "\n"], acks
+  rescue Timeout::Error
+    flunk "Application#serve did not send the no-pid response to the manager " \
+          "— ApplicationManager#run will block on child.gets.to_i and deadlock " \
+          "the server's accept loop"
+  end
+end

test/unit/server_test.rb

@@ -0,0 +1,52 @@
+require_relative "../helper"
+require "spring/env"
+require "spring/server"
+require "fileutils"
+require "tmpdir"
+
+# Regression test for https://github.com/rails/spring/issues/724:
+# disconnected client raises Errno::EPIPE in #serve, which used to crash
+# the accept loop and require `spring stop` to recover.
+class ServerTest < ActiveSupport::TestCase
+  def setup
+    @tmpdir   = Dir.mktmpdir("spring-server-test")
+    @log_file = File.open(File::NULL, "a")
+    @env      = Spring::Env.new(log_file: @log_file)
+    # Pin the env's pidfile path to the test's tmpdir so we don't pollute
+    # the user's spring tmp directory.
+    pidfile_path = Pathname.new(File.join(@tmpdir, "spring.pid"))
+    @env.define_singleton_method(:pidfile_path) { pidfile_path }
+  end
+
+  def teardown
+    @log_file&.close
+    FileUtils.remove_entry(@tmpdir) if @tmpdir
+  end
+
+  test "#serve does not raise when the client disconnects before the version banner is sent" do
+    server = Spring::Server.new(env: @env)
+    server_sock, client_sock = UNIXSocket.pair
+    client_sock.close
+
+    assert_nothing_raised do
+      with_saved_stdio { server.serve(server_sock) }
+    end
+  ensure
+    server_sock&.close
+  end
+
+  private
+
+  # Server#serve calls `redirect_output` in `ensure`, which reopens
+  # STDOUT/STDERR to env.log_file. Save and restore the test runner's
+  # streams around the call so subsequent tests still log normally.
+  def with_saved_stdio
+    saved_out = STDOUT.dup
+    saved_err = STDERR.dup
+    yield
+  ensure
+    STDOUT.reopen(saved_out) if saved_out
+    STDERR.reopen(saved_err) if saved_err
+    [saved_out, saved_err].compact.each { |io| io.close rescue nil }
+  end
+end