prefork MPM: fix "httpd -k restart" leaving the server unable to serve

README.CHANGES

@@ -10,6 +10,11 @@ This covers any developer-visible changes such as a new module API,
 but e.g. code cleanups which don't have any externally-visible effect
 do not need to be documented in CHANGES.
 
+CHANGES records what changed between releases, so a bug which was both
+introduced and fixed since the last release needs no entry.  trunk is
+never released, so fixing a regression which only ever existed on trunk
+needs none at all.
+
 Changes should be documented by creating a file in changes-entries/
 with the .txt suffix, using the following template:
 
@@ -21,6 +26,9 @@ with the .txt suffix, using the following template:
 
 Changes to server/*.[ch] use a "core:" prefix rather than "mod_foo:".
 
+Changes to an MPM are named for the MPM rather than the module which
+implements it, e.g. "prefork MPM:" rather than "mpm_prefork:".
+
 The description should be as concise as possible, a maximum of three
 lines but ideally one or two; describe the user-visible effect of the
 change rather than simply describing how the code was fixed. New

server/mpm/prefork/prefork.c

@@ -970,7 +970,11 @@ static int prefork_run(apr_pool_t *_pconf, apr_pool_t *plog, server_rec *s)
             }
         }
         else {
-            /* Kill 'em off */
+            /* Kill 'em off.  The parent is in this process group too, and
+             * its handler would take the signal as a fresh restart request
+             * and loop; ap_unixd_mpm_set_signals() below puts the handler
+             * back. */
+            apr_signal(SIGHUP, SIG_IGN);
             if (ap_unixd_killpg(getpgrp(), SIGHUP) < 0) {
                 ap_log_error(APLOG_MARK, APLOG_WARNING, errno,
                              ap_server_conf, APLOGNO(00172) "killpg SIGHUP");

test/modules/arch/linux/env.py

@@ -8,7 +8,6 @@
 import subprocess
 import threading
 import time
-from datetime import timedelta
 from typing import Callable, Dict, List, Optional
 
 from pyhttpd.env import HttpdTestEnv, HttpdTestSetup
@@ -81,25 +80,6 @@ def server_env(self) -> Dict[str, str]:
     def httpd_bin(self) -> str:
         return os.path.join(self.bin_dir, 'httpd')
 
-    def apache_hard_restart(self) -> int:
-        """Restart without the "graceful" flag, so the MPM starts over."""
-        r = self._run_apachectl("restart")
-        if r.exit_code == 0:
-            return 0 if self.is_live(self._http_base, timeout=timedelta(seconds=10)) else -1
-        return r.exit_code
-
-    def read_pid_file(self, name: str = 'httpd.pid') -> Optional[int]:
-        # Where PidFile lands depends on how the httpd under test resolves
-        # a relative path against DefaultRuntimeDir, which has differed
-        # between versions; look in both places rather than assume.
-        for d in (self.server_logs_dir, self.server_dir):
-            try:
-                with open(os.path.join(d, name)) as fd:
-                    return int(fd.read().strip())
-            except (OSError, ValueError):
-                continue
-        return None
-
 
 class NotifyListener:
     """A stand-in for the systemd notification socket.

test/modules/core/test_008_restart.py

@@ -0,0 +1,54 @@
+import pytest
+
+from pyhttpd.conf import HttpdConf
+
+
+class TestRestart:
+    """The server goes on serving across a restart, graceful or not.
+
+    Whatever the MPM does with its children, what has to hold afterwards
+    is that a request is answered.  A restart which leaves the listening
+    socket open with nothing behind it is the failure this catches, and
+    it is not visible from the exit status of "httpd -k restart".
+    """
+
+    @pytest.fixture(autouse=True, scope='class')
+    def _class_scope(self, env):
+        conf = HttpdConf(env)
+        conf.add_vhost_test1()
+        conf.install()
+        # Restarting is rough on mod_cgid, which is loaded for this
+        # package: the daemon is signalled along with the rest of the
+        # process group (AH01239), and restarting again before it has
+        # unlinked its socket leaves the new one unable to bind
+        # (AH01243).  Neither is what these tests are about.
+        env.httpd_error_log.add_ignored_lognos(['AH01239', 'AH01243'])
+        assert env.apache_restart() == 0
+
+    def get(self, env, when):
+        # --max-time, or a server which accepts the connection and then
+        # says nothing hangs here rather than failing.
+        r = env.curl_get(env.mkurl("http", "test1", "/"),
+                         options=['--max-time', '10'])
+        assert r.exit_code == 0, f"no answer {when}: {r.stderr}"
+        assert r.response['status'] == 200, f"bad status {when}"
+
+    def test_core_008_01_graceful(self, env):
+        self.get(env, "before the reload")
+        assert env.apache_reload() == 0
+        self.get(env, "after a graceful reload")
+
+    def test_core_008_02_ungraceful(self, env):
+        """"httpd -k restart" keeps the same parent and serves again."""
+        self.get(env, "before the restart")
+        pid = env.read_pid_file()
+        assert pid, "no pid file"
+        assert env.apache_hard_restart() == 0
+        assert env.read_pid_file() == pid, "the restart replaced the parent"
+        self.get(env, "after an ungraceful restart")
+
+    def test_core_008_03_repeated(self, env):
+        """Restarting twice in a row is no different from once."""
+        for _ in range(2):
+            assert env.apache_hard_restart() == 0
+            self.get(env, "after repeated restarts")

test/pyhttpd/env.py

@@ -848,6 +848,26 @@ def apache_fail(self):
             rv = 0
         return rv
 
+    def apache_hard_restart(self) -> int:
+        """Restart without the "graceful" flag, so the MPM starts over."""
+        r = self._run_apachectl("restart")
+        if r.exit_code == 0:
+            return 0 if self.is_live(self._http_base,
+                                     timeout=timedelta(seconds=10)) else -1
+        return r.exit_code
+
+    def read_pid_file(self, name: str = 'httpd.pid') -> Optional[int]:
+        # Where PidFile lands depends on how the httpd under test resolves
+        # a relative path against DefaultRuntimeDir, which has differed
+        # between versions; look in both places rather than assume.
+        for d in (self._server_logs_dir, self._server_dir):
+            try:
+                with open(os.path.join(d, name)) as fd:
+                    return int(fd.read().strip())
+            except (OSError, ValueError):
+                continue
+        return None
+
     def apache_access_log_clear(self):
         if os.path.isfile(self._server_access_log):
             os.remove(self._server_access_log)