Don't close pipes used for signal handlers in MT

[waj]

This should fix #8375

In MT mode, fork is disabled and the event loop is not reinitialized. In other words, fork only works to run a process (i.e: do an exec) right away, without any context switch or IO operations in between.

[ysbaddaden]

Whatever that fork is disabled in MT: we still fork/exec to run executables, and we must reset signals to SIG_DFL and close the IO pipe in the forked process before calling exec.

[bcardiff]

@ysbaddaden you mean to extract these lines e5b401f#diff-d7e78211632011c14cdae00909be40ddR239-R241 without the pipe close, right?

It would be good to have at least a manual spec to ensure the behavior of signals is not broken. Otherwise it is doing changes in the dark. Do you have some use case/example to stress that signals are handled as expected?

[ysbaddaden]

Actually, not closing the reader part of the pipe fixes #8375 for me. I don't know why, but it's okayish: even if the child process receives a signal it won't write it to the pipe (the writer part is closed) and the reader file descriptor will be closed by exec since IO.pipe already set CLOEXEC on it.

diff --git a/src/signal.cr b/src/signal.cr
index f4c094f25..1737b85a0 100644
--- a/src/signal.cr
+++ b/src/signal.cr
@@ -242,7 +242,7 @@ module Crystal::Signal
       LibC.signal(signal, LibC::SIG_DFL) if signal.set?
     end
   ensure
-    @@pipe.each(&.close)
+    writer.close
   end
 
   private def self.reader

NOTE: we disable signals for the current thread (sigmask) for reasons explained in comments and in d195adf, 979a528 and e5b401f. We must reenable signals prior to exec (otherwise the exec'd process would have disabled signals by default, which is unexpected) and should reset signal handlers before that, so we don't handle signals in the forked process (until exec).

[waj]

@ysbaddaden The signals are still being reset and the pipes have close_on_exec set by default. I just checked that subprocesses doesn't have extra file descriptors opened when they run.

[waj]

Correction... @ysbaddaden you're right, we need to reenable the signals. I'll move the fix into the Signal class.

[ysbaddaden]

Yes.

The sigset is updated before changing the signal handler so I think we don't even need to ever close the pipe (even without MT). LLVM could reorder instructions and cause the signal handler to be set before the sigset, but the likelihood for a thread to fork while another adds a signal handler and the forked child to receive a signal (sending it to the parent) before exec is very very tiny —and impossible with a single thread anyway.