Skip to content

Commit

Permalink
Revert "Fix child processes not being reaped when Process.detach us…
Browse files Browse the repository at this point in the history
…ed (puma#3314)"

This reverts commit 9bd838b.

Did this start to happen after this commit? Sure looks like that so far
https://github.com/dentarg/puma/actions/runs/7709969145/job/21012318760#step:10:43
  • Loading branch information
dentarg committed Jan 30, 2024
1 parent 9bd838b commit cc62233
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 47 deletions.
9 changes: 3 additions & 6 deletions lib/puma/cluster.rb
Original file line number Diff line number Diff line change
Expand Up @@ -563,12 +563,9 @@ def wait_workers
@workers.reject! do |w|
next false if w.pid.nil?
begin
# We may need to check the PID individually because:
# 1. From Ruby versions 2.6 to 3.2, `Process.detach` can prevent or delay
# `Process.wait2(-1)` from detecting a terminated process: https://bugs.ruby-lang.org/issues/19837.
# 2. When `fork_worker` is enabled, some worker may not be direct children,
# but grand children. Because of this they won't be reaped by `Process.wait2(-1)`.
if reaped_children.delete(w.pid) || Process.wait(w.pid, Process::WNOHANG)
# When `fork_worker` is enabled, some worker may not be direct children, but grand children.
# Because of this they won't be reaped by `Process.wait2(-1)`, so we need to check them individually)
if reaped_children.delete(w.pid) || (@options[:fork_worker] && Process.wait(w.pid, Process::WNOHANG))
true
else
w.term if w.term?
Expand Down
11 changes: 0 additions & 11 deletions test/config/process_detach_before_fork.rb

This file was deleted.

32 changes: 2 additions & 30 deletions test/test_integration_cluster.rb
Original file line number Diff line number Diff line change
Expand Up @@ -175,34 +175,6 @@ def test_stuck_external_term_spawn
end
end

# From Ruby 2.6 to 3.2, `Process.detach` can delay or prevent
# `Process.wait2(-1)` from detecting a terminated child:
# https://bugs.ruby-lang.org/issues/19837. However,
# `Process.wait2(<child pid>)` still works properly. This bug has
# been fixed in Ruby 3.3.
def test_workers_respawn_with_process_detach
skip_unless_signal_exist? :KILL

config = 'test/config/process_detach_before_fork.rb'

worker_respawn(0, workers, config) do |phase0_worker_pids|
phase0_worker_pids.each do |pid|
Process.kill :KILL, pid
end
end

# `test/config/process_detach_before_fork.rb` forks and detaches a
# process. Since MiniTest attempts to join all threads before
# finishing, terminate the process so that the test can end quickly
# if it passes.
pid_filename = File.join(Dir.tmpdir, 'process_detach_test.pid')
if File.exist?(pid_filename)
pid = File.read(pid_filename).chomp.to_i
File.unlink(pid_filename)
Process.kill :TERM, pid if pid > 0
end
end

# mimicking stuck workers, test restart
def test_stuck_phased_restart
skip_unless_signal_exist? :USR1
Expand Down Expand Up @@ -709,10 +681,10 @@ def usr1_all_respond(unix: false, config: '')
end
end

def worker_respawn(phase = 1, size = workers, config = 'test/config/worker_shutdown_timeout_2.rb')
def worker_respawn(phase = 1, size = workers)
threads = []

cli_server "-w #{workers} -t 1:1 -C #{config} test/rackup/sleep_pid.ru"
cli_server "-w #{workers} -t 1:1 -C test/config/worker_shutdown_timeout_2.rb test/rackup/sleep_pid.ru"

# make sure two workers have booted
phase0_worker_pids = get_worker_pids
Expand Down

0 comments on commit cc62233

Please sign in to comment.