Allow for setpgid to be configurable

[HeyNonster]

It seems that Process.setpgid moves worker process out of the foreground process group of the controlling terminal, breaking debuggers or otherwise 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, without setpgid both pitchfork processes are in S+ state.

From man ps, 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

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.

I believe it's safe to stop calling setpgid because, as far as I can tell, Pitchfork does not use the pgid of a process for anything (yet).

[HeyNonster]

The smallest way I can reproduce the problem:

# Bad
ruby -ve "if pid = Process.fork;Process.setpgid(pid, pid);Process.wait;else;gets;end"

# Works
ruby -ve "if pid = Process.fork;Process.wait;else;gets;end"

[HeyNonster]

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

[byroot]

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.

[HeyNonster]

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

[byroot]

LGTM.

cc @etiennebarrie could you merge this please? (unless @HeyNonster whants more time to think about the config name)

[HeyNonster]

LGTM.

cc @etiennebarrie could you merge this please? (unless @HeyNonster whants more time to think about the config name)

Nah, it should be fine!

Thanks for your help!