Factor signal handling logic out of ReactorBase (and subclasses)

ReactorBase now sets up a `SignalHandling` object with this behavior.
PosixReactorBase overrides the factory for this object to add SIGCHLD-handling
behavior.

7812431 provides the original implementation
of the glib bits (thanks glyph).  It looks to me like we don't need to do *as
much* work as that revision does to make glib happy - but all I have to go on
is the test suite.

src/twisted/internet/_glibbase.py

@@ -13,14 +13,18 @@
 
 
 import sys
-from typing import Any, Callable, Dict, Set
+from types import TracebackType
+from typing import Any, Callable, Dict, Optional, Set, Type
 
 from zope.interface import implementer
 
+from typing_extensions import Literal
+
 from twisted.internet import posixbase, selectreactor
 from twisted.internet.abstract import FileDescriptor
 from twisted.internet.interfaces import IReactorFDSet, IReadDescriptor, IWriteDescriptor
 from twisted.python import log
+from twisted.python.monkey import MonkeyPatcher
 from ._signals import _UnixWaker
 
 
@@ -65,6 +69,44 @@ def doRead(self) -> None:
         self.reactor._simulate()
 
 
+class _SignalGlue:
+    """
+    Integrate glib's wakeup file descriptor usage and our own.
+
+    Python supports only one wakeup file descriptor at a time and both Twisted
+    and glib want to use it.
+
+    This is a context manager that can be wrapped around the whole glib
+    reactor main loop which makes our signal handling work with glib's signal
+    handling.
+    """
+
+    def __enter__(self) -> None:
+        """
+        Disable glib's use of Python's wakeup fd feature.
+
+        Since we are going to install our own wakeup fd we don't need glib's.
+        This tricks glib into thinking it has installed its already.
+        """
+        from gi import _ossighelper as signalGlue  # type: ignore[import]
+
+        self._patcher = MonkeyPatcher()
+        self._patcher.addPatch(signalGlue, "_wakeup_fd_is_active", True)
+        self._patcher.patch()
+
+    def __exit__(
+        self,
+        excType: Optional[Type[Exception]],
+        excValue: Optional[Exception],
+        traceback: Optional[TracebackType],
+    ) -> Literal[False]:
+        """
+        Restore the original internal glib state.
+        """
+        self._patcher.restore()
+        return False
+
+
 def _loopQuitter(
     idleAdd: Callable[[Callable[[], None]], None], loopQuit: Callable[[], None]
 ) -> Callable[[], None]:
@@ -131,14 +173,18 @@ def __init__(self, glib_module: Any, gtk_module: Any, useGtk: bool = False) -> N
         self._crash = _loopQuitter(self._glib.idle_add, self.loop.quit)
         self._run = self.loop.run
 
-    def _handleSignals(self):
+    def _reallyStartRunning(self):
         # First, install SIGINT and friends:
-        super()._handleSignals()
+        super()._reallyStartRunning()
         # Next, since certain versions of gtk will clobber our signal handler,
         # set all signal handlers again after the event loop has started to
-        # ensure they're *really* set. We don't call this twice so we don't
-        # leak file descriptors created in the SIGCHLD initialization:
-        self.callLater(0, posixbase.PosixReactorBase._handleSignals, self)
+        # ensure they're *really* set.
+
+        def reinitSignals():
+            self._signals.uninstall()
+            self._signals.install()
+
+        self.callLater(0, reinitSignals)
 
     # The input_add function in pygtk1 checks for objects with a
     # 'fileno' method and, if present, uses the result of that method
@@ -278,10 +324,11 @@ def run(self, installSignalHandlers=True):
         """
         Run the reactor.
         """
-        self.callWhenRunning(self._reschedule)
-        self.startRunning(installSignalHandlers=installSignalHandlers)
-        if self._started:
-            self._run()
+        with _SignalGlue():
+            self.callWhenRunning(self._reschedule)
+            self.startRunning(installSignalHandlers=installSignalHandlers)
+            if self._started:
+                self._run()
 
     def callLater(self, *args, **kwargs):
         """

src/twisted/internet/_signals.py

@@ -18,37 +18,45 @@
 API) may not wake up leading to an arbitrary delay before the child
 termination is noticed.
 
-The basic solution to all these issues involves enabling SA_RESTART
-(ie, disabling system call interruption) and registering a C signal
-handler which writes a byte to a pipe.  The other end of the pipe is
-registered with the event loop, allowing it to wake up shortly after
-SIGCHLD is received.  See L{twisted.internet.posixbase._SIGCHLDWaker}
-for the implementation of the event loop side of this solution.  The
-use of a pipe this way is known as the U{self-pipe
+The basic solution to all these issues involves enabling SA_RESTART (ie,
+disabling system call interruption) and registering a C signal handler which
+writes a byte to a pipe.  The other end of the pipe is registered with the
+event loop, allowing it to wake up shortly after SIGCHLD is received.  See
+L{_SIGCHLDWaker} for the implementation of the event loop side of this
+solution.  The use of a pipe this way is known as the U{self-pipe
 trick<http://cr.yp.to/docs/selfpipe.html>}.
 
 From Python version 2.6, C{signal.siginterrupt} and C{signal.set_wakeup_fd}
 provide the necessary C signal handler which writes to the pipe to be
 registered with C{SA_RESTART}.
 """
 
+from __future__ import annotations
 
 import contextlib
 import errno
 import os
 import signal
 import socket
+from types import FrameType
+from typing import Callable, Optional, Protocol, Sequence
 
 from zope.interface import Attribute, Interface, implementer
 
+from attrs import define, frozen
+from typing_extensions import TypeAlias
+
+from twisted.internet.interfaces import IReadDescriptor
 from twisted.python import failure, log, util
 from twisted.python.runtime import platformType
 
 if platformType == "posix":
     from . import fdesc, process
 
+SignalHandler: TypeAlias = Callable[[int, Optional[FrameType]], None]
+
 
-def installHandler(fd):
+def installHandler(fd: int) -> int:
     """
     Install a signal handler which will write a byte to C{fd} when
     I{SIGCHLD} is received.
@@ -59,7 +67,8 @@ def installHandler(fd):
 
     @param fd: The file descriptor to which to write when I{SIGCHLD} is
         received.
-    @type fd: C{int}
+
+    @return: The file descriptor previously configured for this use.
     """
     if fd == -1:
         signal.signal(signal.SIGCHLD, signal.SIG_DFL)
@@ -80,6 +89,178 @@ def isDefaultHandler():
     return signal.getsignal(signal.SIGCHLD) == signal.SIG_DFL
 
 
+class SignalHandling(Protocol):
+    """
+    The L{SignalHandling} protocol enables customizable signal-handling
+    behaviors for reactors.
+
+    A value that conforms to L{SignalHandling} has install and uninstall hooks
+    that are called by a reactor at the correct times to have the (typically)
+    process-global effects necessary for dealing with signals.
+    """
+
+    def install(self) -> None:
+        """
+        Install the signal handlers.
+        """
+
+    def uninstall(self) -> None:
+        """
+        Restore signal handlers to their original state.
+        """
+
+
+@frozen
+class _WithoutSignalHandling:
+    """
+    A L{SignalHandling} implementation that does no signal handling.
+
+    This is the implementation of C{installSignalHandlers=False}.
+    """
+
+    def install(self) -> None:
+        """
+        Do not install any signal handlers.
+        """
+
+    def uninstall(self) -> None:
+        """
+        Do nothing because L{install} installed nothing.
+        """
+
+
+@frozen
+class _WithSignalHandling:
+    """
+    A reactor core helper that can manage signals: it installs signal handlers
+    at start time.
+    """
+
+    _sigInt: SignalHandler
+    _sigBreak: SignalHandler
+    _sigTerm: SignalHandler
+
+    def install(self) -> None:
+        """
+        Install the signal handlers for the Twisted event loop.
+        """
+        try:
+            import signal
+        except ImportError:
+            log.msg(
+                "Warning: signal module unavailable -- "
+                "not installing signal handlers."
+            )
+            return
+
+        if signal.getsignal(signal.SIGINT) == signal.default_int_handler:
+            # only handle if there isn't already a handler, e.g. for Pdb.
+            signal.signal(signal.SIGINT, self._sigInt)
+        signal.signal(signal.SIGTERM, self._sigTerm)
+
+        # Catch Ctrl-Break in windows
+        SIGBREAK = getattr(signal, "SIGBREAK", None)
+        if SIGBREAK is not None:
+            signal.signal(SIGBREAK, self._sigBreak)
+
+    def uninstall(self) -> None:
+        """
+        Do nothing for historical reasons.
+        """
+        # TODO Make this do something, someday, because cleaning up your state
+        # is a nice idea.
+
+
+@define
+class _MultiSignalHandling:
+    """
+    An implementation of L{SignalHandling} which propagates protocol
+    method calls to a number of other implementations.
+
+    This supports composition of multiple signal handling implementations into
+    a single object so the reactor doesn't have to be concerned with how those
+    implementations are factored.
+
+    @ivar _installed: If L{install} has been called but L{uninstall} has not.
+        This is used to avoid double cleanup which otherwise results (at least
+        during test suite runs) because twisted.internet.reactormixins doesn't
+        keep track of whether a reactor has run or not but always invokes its
+        cleanup logic.
+    """
+
+    _delegates: Sequence[SignalHandling]
+    _installed: bool = False
+
+    def install(self) -> None:
+        for d in self._delegates:
+            d.install()
+        self._installed = True
+
+    def uninstall(self) -> None:
+        if self._installed:
+            for d in self._delegates:
+                d.uninstall()
+            self._installed = False
+
+
+@define
+class _ChildSignalHandling:
+    """
+    Signal handling behavior which supports I{SIGCHLD} for notification about
+    changes to child process state.
+
+    @ivar _childWaker: L{None} or a reference to the L{_SIGCHLDWaker} which is
+        used to properly notice child process termination.  This is L{None}
+        when this handling behavior is not installed and non-C{None}
+        otherwise.  This is mostly an unfortunate implementation detail due to
+        L{_SIGCHLDWaker} allocating file descriptors as a side-effect of its
+        initializer.
+    """
+
+    _addInternalReader: Callable[[IReadDescriptor], object]
+    _removeInternalReader: Callable[[IReadDescriptor], object]
+    _childWaker: Optional[_SIGCHLDWaker] = None
+
+    def install(self) -> None:
+        """
+        Extend the basic signal handling logic to also support handling
+        SIGCHLD to know when to try to reap child processes.
+        """
+        self._childWaker = _SIGCHLDWaker()
+        self._addInternalReader(self._childWaker)
+        self._childWaker.install()
+
+        # Also reap all processes right now, in case we missed any
+        # signals before we installed the SIGCHLD waker/handler.
+        # This should only happen if someone used spawnProcess
+        # before calling reactor.run (and the process also exited
+        # already).
+        process.reapAllProcesses()
+
+    def uninstall(self) -> None:
+        """
+        If a child waker was created and installed, uninstall it now.
+
+        Since this disables reactor functionality and is only called when the
+        reactor is stopping, it doesn't provide any directly useful
+        functionality, but the cleanup of reactor-related process-global state
+        that it does helps in unit tests involving multiple reactors and is
+        generally just a nice thing.
+        """
+        assert self._childWaker is not None
+
+        # XXX This would probably be an alright place to put all of the
+        # cleanup code for all internal readers (here and in the base class,
+        # anyway).  See #3063 for that cleanup task.
+        self._removeInternalReader(self._childWaker)
+        self._childWaker.uninstall()
+        self._childWaker.connectionLost(failure.Failure(Exception("uninstalled")))
+
+        # We just spoiled the current _childWaker so throw it away.  We can
+        # make a new one later if need be.
+        self._childWaker = None
+
+
 class _IWaker(Interface):
     """
     Interface to wake up the event loop based on the self-pipe trick.
@@ -161,6 +342,7 @@ def connectionLost(self, reason):
         self.w.close()
 
 
+@implementer(IReadDescriptor)
 class _FDWaker(log.Logger):
     """
     The I{self-pipe trick<http://cr.yp.to/docs/selfpipe.html>}, used to wake
@@ -178,8 +360,8 @@ class _FDWaker(log.Logger):
 
     disconnected = 0
 
-    i = None
-    o = None
+    i: int
+    o: int
 
     def __init__(self):
         """Initialize."""
@@ -190,7 +372,7 @@ def __init__(self):
         fdesc._setCloseOnExec(self.o)
         self.fileno = lambda: self.i
 
-    def doRead(self):
+    def doRead(self) -> None:
         """
         Read some bytes from the pipe and discard them.
         """
@@ -239,8 +421,7 @@ def wakeUp(self):
 
 class _SIGCHLDWaker(_FDWaker):
     """
-    L{_SIGCHLDWaker} can wake up a reactor whenever C{SIGCHLD} is
-    received.
+    L{_SIGCHLDWaker} can wake up a reactor whenever C{SIGCHLD} is received.
     """
 
     def install(self) -> None:
@@ -264,5 +445,5 @@ def doRead(self) -> None:
         writeable, which happens soon after any call to the C{wakeUp}
         method.
         """
-        _FDWaker.doRead(self)
+        super().doRead()
         process.reapAllProcesses()