Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (hermes.apache.org: domain of jorton@redhat.com designates
 66.187.233.31 as permitted sender)
Date: Fri, 20 May 2005 12:22:54 +0100
From: Joe Orton <jorton@redhat.com>
To: dev@httpd.apache.org
Subject: [PATCH] stop killing piped loggers
Message-ID: <20050520112254.GB18240@redhat.com>
Mail-Followup-To: dev@httpd.apache.org
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
User-Agent: Mutt/1.4.1i

As discussed previously; this patch stops killing piped loggers; except
that prefork still kills them at shutdown and ungraceful restart since
it signals the entire process group, so it's not entirely consistent.

In all other cases (graceful restart with prefork; shutdown and both 
types of restart with worker and derivatives): 

- piped loggers are not explicitly killed.  Once all the pipes to the 
piped logger are closed, the logger will read EOF from stdin and know to 
exit.  This is subtle interface change for piped loggers.

- piped loggers will survive across generations; you may have several
sets running concurrently

The effect is that during a graceful restart, log entries processed by
lingering children are correctly handled by piped loggers.  In all
previous releases, they are just thrown away.  I think the importance of
this issue warrants the interface change for 2.2.

Index: server/log.c
===================================================================
--- server/log.c	(revision 170945)
+++ server/log.c	(working copy)
@@ -819,7 +819,6 @@
                                  NULL, procattr, pl->p);
 
         if (status == APR_SUCCESS) {
-            pl->pid = procnew;
             /* procnew->in was dup2'd from ap_piped_log_write_fd(pl);
              * since the original fd is still valid, close the copy to
              * avoid a leak. */
@@ -851,9 +850,6 @@
     switch (reason) {
     case APR_OC_REASON_DEATH:
     case APR_OC_REASON_LOST:
-        pl->pid = NULL; /* in case we don't get it going again, this
-                         * tells other logic not to try to kill it
-                         */
         apr_proc_other_child_unregister(pl);
         stats = ap_mpm_query(AP_MPMQ_MPM_STATE, &mpm_state);
         if (stats != APR_SUCCESS) {
@@ -880,13 +876,12 @@
     case APR_OC_REASON_UNWRITABLE:
         /* We should not kill off the pipe here, since it may only be full.
          * If it really is locked, we should kill it off manually. */
-    break;
+        break;
 
     case APR_OC_REASON_RESTART:
-        if (pl->pid != NULL) {
-            apr_proc_kill(pl->pid, SIGTERM);
-            pl->pid = NULL;
-        }
+        /* At restart time, just forget about existing piped loggers.
+         * When the last child closes the pipe, the logger will read
+         * EOF and exit. */
         break;
 
     case APR_OC_REASON_UNREGISTER:
@@ -895,27 +890,14 @@
 }
 
 
-static apr_status_t piped_log_cleanup_for_exec(void *data)
+static apr_status_t piped_log_cleanup(void *data)
 {
     piped_log *pl = data;
-
     apr_file_close(ap_piped_log_read_fd(pl));
     apr_file_close(ap_piped_log_write_fd(pl));
     return APR_SUCCESS;
 }
 
-
-static apr_status_t piped_log_cleanup(void *data)
-{
-    piped_log *pl = data;
-
-    if (pl->pid != NULL) {
-        apr_proc_kill(pl->pid, SIGTERM);
-    }
-    return piped_log_cleanup_for_exec(data);
-}
-
-
 AP_DECLARE(piped_log *) ap_open_piped_log(apr_pool_t *p, const char *program)
 {
     piped_log *pl;
@@ -928,8 +910,7 @@
                              &ap_piped_log_write_fd(pl), p) != APR_SUCCESS) {
         return NULL;
     }
-    apr_pool_cleanup_register(p, pl, piped_log_cleanup,
-                              piped_log_cleanup_for_exec);
+    apr_pool_cleanup_register(p, pl, piped_log_cleanup, piped_log_cleanup);
     if (piped_log_spawn(pl) != APR_SUCCESS) {
         apr_pool_cleanup_kill(p, pl, piped_log_cleanup);
         apr_file_close(ap_piped_log_read_fd(pl));