Skip to content
New issue

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

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

Already on GitHub? # to your account

Allow for setpgid to be configurable #148

Merged
merged 1 commit into from
Jan 17, 2025
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions docs/CONFIGURATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,18 @@ The following options may be specified (but are generally not needed):

Default: `queues - 1`.

- `setpgid: true or false`

When enabled, child processes will be made into a group leader with a pgid matching
their pid. The child process will no longer be in the foreground process group of
its control terminal.

This can break debugging sessions and interactive terminals inside the worker
processes. Set it to `false` if you need to activate a debugging session or read from
`STDIN` inside a worker process.

Default: `true` (enabled)

- `umask: mode`

Sets the file mode creation mask for UNIX sockets.
Expand Down
5 changes: 5 additions & 0 deletions lib/pitchfork/configurator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class Configurator
:client_body_buffer_size => Pitchfork::Const::MAX_BODY,
:before_service_worker_ready => nil,
:before_service_worker_exit => nil,
:setpgid => false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to use a different key/method name here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't know. At least it's pretty clear which syscall is involved. But people may think they're meant to give a numeric ID or something.

Honestly I think it's fine, but if you have a better idea let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine, tbh, but wanted to mention it just in case.

}
#:startdoc:

Expand Down Expand Up @@ -207,6 +208,10 @@ def timeout_signal(*args, &block)
end
end

def setpgid(bool)
set_bool(:setpgid, bool)
end

def spawn_timeout(seconds)
set_int(:spawn_timeout, seconds, 1)
end
Expand Down
8 changes: 4 additions & 4 deletions lib/pitchfork/http_server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def extend_deadline(extra_time)
attr_accessor :app, :timeout, :timeout_signal, :soft_timeout, :cleanup_timeout, :spawn_timeout, :worker_processes,
:before_fork, :after_worker_fork, :after_mold_fork, :before_service_worker_ready, :before_service_worker_exit,
:listener_opts, :children,
:orig_app, :config, :ready_pipe, :early_hints
:orig_app, :config, :ready_pipe, :early_hints, :setpgid
attr_writer :after_worker_exit, :before_worker_exit, :after_worker_ready, :after_request_complete,
:refork_condition, :after_worker_timeout, :after_worker_hard_timeout, :after_monitor_ready

Expand Down Expand Up @@ -667,7 +667,7 @@ def spawn_service(service, detach:)
def spawn_initial_mold
mold = Worker.new(nil)
mold.create_socketpair!
mold.pid = Pitchfork.clean_fork do
mold.pid = Pitchfork.clean_fork(setpgid: setpgid) do
mold.pid = Process.pid
@promotion_lock.try_lock
mold.after_fork_in_child
Expand Down Expand Up @@ -1187,7 +1187,7 @@ def fork_sibling(role, &block)
else # first child
r.close
Process.setproctitle("<pitchfork fork_sibling(#{role})>")
pid = Pitchfork.clean_fork do
pid = Pitchfork.clean_fork(setpgid: setpgid) do
# detach into a grand child
w.close
yield
Expand All @@ -1202,7 +1202,7 @@ def fork_sibling(role, &block)
exit!
end
else
Pitchfork.clean_fork(&block)
Pitchfork.clean_fork(setpgid: setpgid, &block)
end
end

Expand Down
42 changes: 42 additions & 0 deletions test/integration/test_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,48 @@ def test_modify_internals
assert_clean_shutdown(pid)
end

def test_setpgid_true
addr, port = unused_port

pid = spawn_server(app: File.join(ROOT, "test/integration/pid.ru"), config: <<~CONFIG)
listen "#{addr}:#{port}"
worker_processes 1

setpgid true
CONFIG

assert_healthy("http://#{addr}:#{port}")

worker_pid = Net::HTTP.get(URI("http://#{addr}:#{port}")).strip.to_i

pgid_pid = Process.getpgid(pid)
pgid_worker = Process.getpgid(worker_pid)

refute_equal(pgid_pid, pgid_worker)
assert_clean_shutdown(pid)
end

def test_setpgid_false
addr, port = unused_port

pid = spawn_server(app: File.join(ROOT, "test/integration/pid.ru"), config: <<~CONFIG)
listen "#{addr}:#{port}"
worker_processes 1

setpgid false
CONFIG

assert_healthy("http://#{addr}:#{port}")

worker_pid = Net::HTTP.get(URI("http://#{addr}:#{port}")).strip.to_i

pgid_pid = Process.getpgid(pid)
pgid_worker = Process.getpgid(worker_pid)

assert_equal(pgid_pid, pgid_worker)
assert_clean_shutdown(pid)
end

def test_at_exit_handlers
addr, port = unused_port

Expand Down