Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In fork_worker mode, worker_culling_strategy "oldest" shouldn't cull worker 0 #2794

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 23 additions & 12 deletions lib/puma/cluster.rb
Expand Up @@ -108,25 +108,36 @@ def spawn_worker(idx, master)
def cull_workers
diff = @workers.size - @options[:workers]
return if diff < 1
debug "Culling #{diff} workers"

debug "Culling #{diff.inspect} workers"
workers = workers_to_cull(diff)
debug "Workers to cull: #{workers.inspect}"

workers_to_cull =
case @options[:worker_culling_strategy]
when :youngest
@workers.sort_by(&:started_at)[-diff,diff]
when :oldest
@workers.sort_by(&:started_at)[0,diff]
end

debug "Workers to cull: #{workers_to_cull.inspect}"

workers_to_cull.each do |worker|
workers.each do |worker|
log "- Worker #{worker.index} (PID: #{worker.pid}) terminating"
worker.term
end
end

def workers_to_cull(diff)
workers = @workers.sort_by(&:started_at)

# In fork_worker mode, worker 0 acts as our master process.
# We should avoid culling it to preserve copy-on-write memory gains.
workers.reject! { |w| w.index == 0 } if @options[:fork_worker]
johnnyshields marked this conversation as resolved.
Show resolved Hide resolved

workers[cull_start_index(diff), diff]
end

def cull_start_index(diff)
case @options[:worker_culling_strategy]
when :oldest
0
else # :youngest
-diff
end
end

# @!attribute [r] next_worker_index
def next_worker_index
occupied_positions = @workers.map(&:index)
Expand Down
19 changes: 19 additions & 0 deletions test/test_integration_cluster.rb
Expand Up @@ -401,6 +401,25 @@ def test_culling_strategy_oldest
assert_match(/Worker 0 \(PID: \d+\) terminating/, line)
end

def test_culling_strategy_oldest_fork_worker
cli_server "-w 2 test/rackup/hello.ru", config: <<RUBY
worker_culling_strategy :oldest
fork_worker
RUBY

get_worker_pids # to consume server logs

Process.kill :TTIN, @pid

line = @server.gets
assert_match(/Worker 2 \(PID: \d+\) booted in/, line)

Process.kill :TTOU, @pid

line = @server.gets
assert_match(/Worker 1 \(PID: \d+\) terminating/, line)
end

private

def worker_timeout(timeout, iterations, details, config)
Expand Down