Skip to content

Commit

Permalink
Allow for setpgid to be configurable
Browse files Browse the repository at this point in the history
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 <signal.h>.

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)
▽#<Thread:0x00000001168d9638@DEBUGGER__::SESSION@server /debug-1.10.0/lib/debug/session.rb:179 run> 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.
  • Loading branch information
HeyNonster committed Jan 17, 2025
1 parent 446d98b commit cc7e109
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 4 deletions.
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,
}
#: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

0 comments on commit cc7e109

Please # to comment.