Refactor listener closing code (#2112)

* Simplify Runner#start_control URL parsing
Reuse logic from Binder#parse.

* Parameterize log message

* Ensure control server Unix socket is closed on shutdown
Remove Binder#close_unix_paths

Co-authored-by: Nate Berkopec <nate.berkopec@gmail.com>

History.md

@@ -13,6 +13,7 @@
 
 * Bugfixes
   * Windows update extconf.rb for use with ssp and varied Ruby/MSYS2 combinations (#2069)
+  * Ensure control server Unix socket is closed on shutdown (#2112)
   * Preserve `BUNDLE_GEMFILE` env var when using `prune_bundler` (#1893)
   * Send 408 request timeout even when queue requests is disabled (#2119)
   * Rescue IO::WaitReadable instead of EAGAIN for blocking read (#2121)

lib/puma/binder.rb

@@ -370,10 +370,6 @@ def close_listeners
       end
     end
 
-    def close_unix_paths
-      @unix_paths.each { |up| File.unlink(up) if File.exist? up }
-    end
-
     def redirects_for_restart
       redirects = {:close_others => true}
       @listeners.each_with_index do |(l, io), i|

lib/puma/launcher.rb

@@ -184,7 +184,7 @@ def run
       when :exit
         # nothing
       end
-      @binder.close_unix_paths
+      close_binder_listeners unless @status == :restart
     end
 
     # Return all tcp ports the launcher may be using, TCP or SSL
@@ -202,6 +202,7 @@ def restart_args
     end
 
     def close_binder_listeners
+      @runner.close_control_listeners
       @binder.close_listeners
     end
 

lib/puma/runner.rb

@@ -68,6 +68,10 @@ def start_control
       @control = control
     end
 
+    def close_control_listeners
+      @control.binder.close_listeners if @control
+    end
+
     def ruby_engine
       if !defined?(RUBY_ENGINE) || RUBY_ENGINE == "ruby"
         "ruby #{RUBY_VERSION}-p#{RUBY_PATCHLEVEL}"

test/test_binder.rb

@@ -90,7 +90,7 @@ def test_pre_existing_unix
 
     refute_includes @binder.instance_variable_get(:@unix_paths), unix_path
 
-    @binder.close_unix_paths
+    @binder.close_listeners
 
     assert File.exist?(unix_path)
 
@@ -150,7 +150,7 @@ def assert_parsing_logs_uri(order = [:unix, :tcp])
     assert stdout.include?(prepared_paths[order[0]]), "\n#{stdout}\n"
     assert stdout.include?(prepared_paths[order[1]]), "\n#{stdout}\n"
   ensure
-    @binder.close_unix_paths if order.include?(:unix) && UNIX_SKT_EXIST
+    @binder.close_listeners if order.include?(:unix) && UNIX_SKT_EXIST
   end
 end
 

test/test_integration_pumactl.rb

@@ -14,6 +14,8 @@ def setup
   def teardown
     super
 
+    refute File.exist?(@control_path), "Control path must be removed after stop"
+  ensure
     [@state_path, @control_path].each { |p| File.unlink(p) rescue nil }
   end
 
@@ -30,10 +32,18 @@ def test_stop_tcp
   end
 
   def test_stop_unix
+    ctl_unix
+  end
+
+  def test_halt_unix
+    ctl_unix 'halt'
+  end
+
+  def ctl_unix(signal='stop')
     skip UNIX_SKT_MSG unless UNIX_SKT_EXIST
     cli_server "-q test/rackup/sleep.ru --control-url unix://#{@control_path} --control-token #{TOKEN} -S #{@state_path}", unix: true
 
-    cli_pumactl "stop", unix: true
+    cli_pumactl signal, unix: true
 
     _, status = Process.wait2(@pid)
     assert_equal 0, status