From cc7e1094b82e723875fe91f80a6c680ade16324d Mon Sep 17 00:00:00 2001 From: Nony Dutton Date: Fri, 17 Jan 2025 13:36:10 +0100 Subject: [PATCH] Allow for `setpgid` to be configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It seems that `Process.setpgid` moves worker process out of the foreground process group of the controlling terminal, breaking debuggers or reading from `STDIN`. An example of the problem with slightly truncated `ps -l` output: ``` \# With Process.setpgid(pid, pid) ps -l UID PID PPID RSS WCHAN S TTY TIME CMD 503 73338 67944 92432 - S+ ttys001 0:00.79 pitchfork monitor - 503 73343 73338 13808 - S ttys001 0:00.00 pitchfork (gen:0) worker[0] - requests: 0, waiting \# Without Process.setpgid(pid, pid) $ ps -l UID PID PPID RSS WCHAN S TTY TIME CMD 503 73358 67944 93264 - S+ ttys001 0:00.81 pitchfork monitor - 503 73363 73358 13248 - S+ ttys001 0:00.00 pitchfork (gen:0) worker[0] - requests: 0, waiting ``` The `S` column is the `state` of the process. You will notice, that without `setpgid` both pitchfork processes are in `S+` state. From [man ps](https://man7.org/linux/man-pages/man1/ps.1.html), `PROCESS STATE CODES` section: > S interruptible sleep (waiting for an event to complete) > + is in the foreground process group From [General Terminal Interface, 11.1.4 Terminal Access Control](https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap11.html) > If a process is in the foreground process group of its controlling terminal, read operations shall be allowed, as described in Input Processing and Reading Data. Any attempts by a process in a background process group to read from its controlling terminal cause its process group to be sent a SIGTTIN signal unless one of the following special cases applies: if the reading process is ignoring the SIGTTIN signal or the reading thread is blocking the SIGTTIN signal, or if the process group of the reading process is orphaned, the read() shall return -1, with errno set to [EIO] and no signal shall be sent. The default action of the SIGTTIN signal shall be to stop the process to which it is sent. See . When the worker process is not in the foreground process group we get an `Errno::EIO` exception ``` [1, 7] in ~/Code/experiments/pitchfork_debugger/app/controllers/foo_controller.rb 1| class FooController < ApplicationController 2| def index => 3| require 'debug';debugger 4| 5| head :ok 6| end 7| end =>#0 FooController#index at ~/Code/experiments/pitchfork_debugger/app/controllers/foo_controller.rb:3 #1 ActionController::BasicImplicitRender#send_action(method="index", args=[]) at $(Gem)/actionpack-8.0.1/lib/action_controller/metal/basic_implicit_render.rb:8 # and 100 frames (use `bt' command for all frames) ▽# terminated with exception (report_on_exception is true): /reline-0.6.0/lib/reline/io/ansi.rb:214:in 'IO#readpartial': Input/output error (Errno::EIO) ``` This commit adds a `setpgid` configuration option that allows you to set `setpgid` to false, if needed, so that you can open debugging sessions. This is probably only useful in development. --- docs/CONFIGURATION.md | 12 ++++++++ lib/pitchfork/configurator.rb | 5 +++ lib/pitchfork/http_server.rb | 8 ++--- test/integration/test_configuration.rb | 42 ++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 4 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 3613a1a0..8c2371c2 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -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. diff --git a/lib/pitchfork/configurator.rb b/lib/pitchfork/configurator.rb index 6afe06c0..ed1c5b3b 100644 --- a/lib/pitchfork/configurator.rb +++ b/lib/pitchfork/configurator.rb @@ -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, } #:startdoc: @@ -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 diff --git a/lib/pitchfork/http_server.rb b/lib/pitchfork/http_server.rb index 9bca0a21..e55f34fc 100644 --- a/lib/pitchfork/http_server.rb +++ b/lib/pitchfork/http_server.rb @@ -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 @@ -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 @@ -1187,7 +1187,7 @@ def fork_sibling(role, &block) else # first child r.close Process.setproctitle("") - pid = Pitchfork.clean_fork do + pid = Pitchfork.clean_fork(setpgid: setpgid) do # detach into a grand child w.close yield @@ -1202,7 +1202,7 @@ def fork_sibling(role, &block) exit! end else - Pitchfork.clean_fork(&block) + Pitchfork.clean_fork(setpgid: setpgid, &block) end end diff --git a/test/integration/test_configuration.rb b/test/integration/test_configuration.rb index ca27c39c..ae9acd75 100644 --- a/test/integration/test_configuration.rb +++ b/test/integration/test_configuration.rb @@ -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