(this likely applies to some other threaded MPMs)

Sometimes the MPM has to create a new child process even though there
are no free scoreboard slots.  This can occur when child process
currently consuming scoreboard slots are trying to exit, but still
have at least one thread serving an active request.  In this case, the
new child will take over the process slot immediately and take over
thread slots as threads in the old child terminate.  As soon as the
new child takes over the process slot, the MPM forgets that the old
child existed.  If the old child never exits on its own (long-running
or hung request), then when Apache is terminated the old child will
still hang around (parent pid -> 1) and will have to be killed
manually.

The MPM could potentially blow away forgotten children by sending
signals to the process group (wait until the normal termination
sequence is done, then blow away any processes we've forgotten about).

We could rework the scoreboard design to allow the maximum number of
child processes to be represented in the scoreboard directly, though
that may require module changes and potentially an access protocol to
allow thread entries to be moved from one process to another without
crashing any code accessing the scoreboard.

We could add the pid to every worker thread entry in the scoreboard,
then ap_reclaim_child_processes() could use that field to search for
processes that no longer have a process slot but still have active
threads.

In the attached patch I took a different path -- explicitly maintain a
list of processes which no longer have a process slot in the
scoreboard.  ap_reclaim_child_processes() treats these
otherwise-forgotten processes just like the ones which are lucky
enough to have process slots in the scoreboard.  Before making some
minor tweaks it *seemed to work*, but detailed testing hasn't been
done.  At this point I'm curious to hear comments on the design.
Index: server/mpm/worker/worker.c
===================================================================
--- server/mpm/worker/worker.c  (revision 106918)
+++ server/mpm/worker/worker.c  (working copy)
@@ -1310,6 +1310,21 @@
         clean_child_exit(0);
     }
     /* else */
+    if (ap_scoreboard_image->parent[slot].pid != 0) {
+        /* This new child process is squatting on the scoreboard
+         * entry owned by an exiting child process, which cannot
+         * exit until all active requests complete.
+         * Don't forget about this exiting child process, or we
+         * won't be able to kill it if it doesn't exit by the
+         * time the server is shut down.
+         */
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf,
+                     "taking over scoreboard slot from %" APR_PID_T_FMT "%s",
+                     ap_scoreboard_image->parent[slot].pid,
+                     ap_scoreboard_image->parent[slot].quiescing ?
+                         " (quiescing)" : "");
+        ap_remember_extra_mpm_process(ap_scoreboard_image->parent[slot].pid);
+    }
     ap_scoreboard_image->parent[slot].quiescing = 0;
     ap_scoreboard_image->parent[slot].pid = pid;
     return 0;
@@ -1524,6 +1539,9 @@
                     make_child(ap_server_conf, child_slot);
                     --remaining_children_to_start;
                 }
+            }
+            else if (ap_forget_extra_mpm_process(pid.pid) == 1) {
+                /* handled */
 #if APR_HAS_OTHER_CHILD
             }
             else if (apr_proc_other_child_alert(&pid, APR_OC_REASON_DEATH,
Index: server/mpm_common.c
===================================================================
--- server/mpm_common.c (revision 106918)
+++ server/mpm_common.c (working copy)
@@ -59,11 +59,120 @@
 #endif
 
 #ifdef AP_MPM_WANT_RECLAIM_CHILD_PROCESSES
+
+typedef enum {DO_NOTHING, SEND_SIGTERM, SEND_SIGKILL, GIVEUP} action_t;
+
+typedef struct extra_process_t {
+    struct extra_process_t *next;
+    pid_t pid;
+} extra_process_t;
+
+static extra_process_t *extras;
+
+void ap_remember_extra_mpm_process(pid_t pid)
+{
+    extra_process_t *p = (extra_process_t *)malloc(sizeof(extra_process_t));
+
+    p->next = extras;
+    p->pid = pid;
+    extras = p;
+}
+
+int ap_forget_extra_mpm_process(pid_t pid)
+{
+    extra_process_t *cur = extras;
+    extra_process_t *prev = NULL;
+
+    while (cur && cur->pid != pid) {
+        prev = cur;
+        cur = cur->next;
+    }
+
+    if (cur) {
+        if (prev) {
+            prev->next = cur->next;
+        }
+        else {
+            extras = cur->next;
+        }
+        free(cur);
+        return 1; /* found */
+    }
+    else {
+        /* we don't know about any such process */
+        return 0;
+    }
+}
+
+static int reclaim_one_pid(pid_t pid, action_t action)
+{
+    apr_proc_t proc;
+    apr_status_t waitret;
+
+    proc.pid = pid;
+    waitret = apr_proc_wait(&proc, NULL, NULL, APR_NOWAIT);
+    if (waitret != APR_CHILD_NOTDONE) {
+        return 1;
+    }
+
+    switch(action) {
+    case DO_NOTHING:
+        break;
+        
+    case SEND_SIGTERM:
+        /* ok, now it's being annoying */
+        ap_log_error(APLOG_MARK, APLOG_WARNING,
+                     0, ap_server_conf,
+                     "child process %" APR_PID_T_FMT
+                     " still did not exit, "
+                     "sending a SIGTERM",
+                     pid);
+        kill(pid, SIGTERM);
+        break;
+        
+    case SEND_SIGKILL:
+        ap_log_error(APLOG_MARK, APLOG_ERR,
+                     0, ap_server_conf,
+                     "child process %" APR_PID_T_FMT
+                     " still did not exit, "
+                     "sending a SIGKILL",
+                     pid);
+#ifndef BEOS
+        kill(pid, SIGKILL);
+#else
+        /* sending a SIGKILL kills the entire team on BeOS, and as
+         * httpd thread is part of that team it removes any chance
+         * of ever doing a restart.  To counter this I'm changing to
+         * use a kinder, gentler way of killing a specific thread
+         * that is just as effective.
+         */
+        kill_thread(pid);
+#endif
+        break;
+                
+    case GIVEUP:
+        /* gave it our best shot, but alas...  If this really
+         * is a child we are trying to kill and it really hasn't
+         * exited, we will likely fail to bind to the port
+         * after the restart.
+         */
+        ap_log_error(APLOG_MARK, APLOG_ERR,
+                     0, ap_server_conf,
+                     "could not make child process %" APR_PID_T_FMT
+                     " exit, "
+                     "attempting to continue anyway",
+                     pid);
+        break;
+    }
+    
+    return 0;
+}
+
 void ap_reclaim_child_processes(int terminate)
 {
     apr_time_t waittime = 1024 * 16;
-    apr_status_t waitret;
     int i;
+    extra_process_t *cur_extra;
     int not_dead_yet;
     int max_daemons;
     apr_time_t starttime = apr_time_now();
@@ -71,7 +180,7 @@
      * at which elapsed time from starting the reclaim
      */
     struct {
-        enum {DO_NOTHING, SEND_SIGTERM, SEND_SIGKILL, GIVEUP} action;
+        action_t action;
         apr_time_t action_time;
     } action_table[] = {
         {DO_NOTHING, 0}, /* dummy entry for iterations where we reap
@@ -115,70 +224,31 @@
         not_dead_yet = 0;
         for (i = 0; i < max_daemons; ++i) {
             pid_t pid = MPM_CHILD_PID(i);
-            apr_proc_t proc;
 
-            if (pid == 0)
-                continue;
+            if (pid == 0) {
+                continue; /* not every scoreboard entry is in use */
+            }
 
-            proc.pid = pid;
-            waitret = apr_proc_wait(&proc, NULL, NULL, APR_NOWAIT);
-            if (waitret != APR_CHILD_NOTDONE) {
+            if (reclaim_one_pid(pid, action_table[cur_action].action)) {
                 MPM_NOTE_CHILD_KILLED(i);
-                continue;
             }
+            else {
+                ++not_dead_yet;
+            }
+        }
 
-            ++not_dead_yet;
-            switch(action_table[cur_action].action) {
-            case DO_NOTHING:
-                break;
-                
-            case SEND_SIGTERM:
-                /* ok, now it's being annoying */
-                ap_log_error(APLOG_MARK, APLOG_WARNING,
-                             0, ap_server_conf,
-                             "child process %" APR_PID_T_FMT
-                             " still did not exit, "
-                             "sending a SIGTERM",
-                             pid);
-                kill(pid, SIGTERM);
-                break;
-                
-            case SEND_SIGKILL:
-                ap_log_error(APLOG_MARK, APLOG_ERR,
-                             0, ap_server_conf,
-                             "child process %" APR_PID_T_FMT
-                             " still did not exit, "
-                             "sending a SIGKILL",
-                             pid);
-#ifndef BEOS
-                kill(pid, SIGKILL);
-#else
-                /* sending a SIGKILL kills the entire team on BeOS, and as
-                 * httpd thread is part of that team it removes any chance
-                 * of ever doing a restart.  To counter this I'm changing to
-                 * use a kinder, gentler way of killing a specific thread
-                 * that is just as effective.
-                 */
-                kill_thread(pid);
-#endif
-                break;
-                
-            case GIVEUP:
-                /* gave it our best shot, but alas...  If this really
-                 * is a child we are trying to kill and it really hasn't
-                 * exited, we will likely fail to bind to the port
-                 * after the restart.
-                 */
-                ap_log_error(APLOG_MARK, APLOG_ERR,
-                             0, ap_server_conf,
-                             "could not make child process %" APR_PID_T_FMT
-                             " exit, "
-                             "attempting to continue anyway",
-                             pid);
-                break;
+        cur_extra = extras;
+        while (cur_extra) {
+            extra_process_t *next = cur_extra->next;
+
+            if (reclaim_one_pid(cur_extra->pid, 
action_table[cur_action].action)) {
+                AP_DEBUG_ASSERT(1 == 
ap_forget_extra_mpm_process(cur_extra->pid));
             }
+            else {
+                ++not_dead_yet;
+            }
+            cur_extra = next;
         }
-
 #if APR_HAS_OTHER_CHILD
         apr_proc_other_child_refresh_all(APR_OC_REASON_RESTART);
 #endif
Index: include/mpm_common.h
===================================================================
--- include/mpm_common.h        (revision 106918)
+++ include/mpm_common.h        (working copy)
@@ -70,6 +70,8 @@
  */
 #ifdef AP_MPM_WANT_RECLAIM_CHILD_PROCESSES
 void ap_reclaim_child_processes(int terminate);
+void ap_remember_extra_mpm_process(pid_t pid);
+int ap_forget_extra_mpm_process(pid_t pid);
 #endif
 
 /**

Reply via email to