httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Darroch <chr...@pearsoncmg.com>
Subject configuration directives redux
Date Thu, 20 Jul 2006 22:09:52 GMT
Hi --

   Some time ago, I proposed this large patchset (better described,
I think, by the message referenced by the second link below):

http://marc.theaimsgroup.com/?l=apache-httpd-dev&m=114729206702495&w=2
http://marc.theaimsgroup.com/?l=apache-httpd-dev&m=114788040600327&w=2

   Discussing the issues on IRC, I received a few responses, including
a long one from William A. Rowe, Jr.  His main point, I believe,
was that the order of configuration directives in the tree should
never be altered from what it is in the configuration files.  Therefore,
what the worker and event MPMs do now in their pre_config stages --
namely, re-ordering ThreadsPerChild ahead of MaxClients -- should not
be done.  Instead, all such order-dependent configuration directive
checking should be done in a post-config phase, either open_logs or
post_config.

   (Note, BTW, that the existing re-ordering is in fact insufficient,
because there are other dependencies between the four directives in
question: ServerLimit, ThreadLimit, MaxClients, and ThreadsPerChild.
Hence my original patchset proposed a standard ap_swap_nodes()
to perform all the re-ordering that seemed necessary.)


   Thinking about Bill's comments and doing some additional research,
I've come up with the following patch.  So far, I've just been working
on prefork; if it seems sensible, then I'll apply the same ideas to
the winnt, worker, and event MPMs, which all share the same essential
logic when it comes to these particular configuration directives.

   If some people could review this new approach, I'd really
appreciate it, because (among other things) I'd love to get this
item off my plate so I can move on to other stuff.  (I suppose I
could just do that anyway, but to me that feels like cheating.  :-)


   So, to aid with that, a few comments about what I'm attempting
to do in this patch.  First, from the second link above, I'm
trying to achieve items B and C:

> B) A fix for the following less-than-critical issue:
> 
>> I noticed that although there was
>> usually a warning if one tried to change ServerLimit or ThreadLimit
>> and restart the server, no warning appeared if one removed (e.g.,
>> commented out) these directives and restarted.  Presumably,
>> removing these directives implies the use of the default values,
>> but if the server was originally configured with a non-default value,
>> then a warning should probably appear.
> 
> C) Attempting to ensure that after a restart, valid warning messages
>    always appear in the log files about any changes that the code
>    has made to the configured values for ServerLimit, ThreadLimit,
>    MaxClients, and ThreadsPerChild.  Warning messages are printed
>    to the console too, as before, but may not always be valid since
>    the original values of ServerLimit and ThreadLimit aren't
>    known to the process that prints them.

   (Item D doesn't apply anymore, since I'm looking to follow
Bill's advice on that score, and item A doesn't apply to prefork,
only worker and event.  I figured I'd start with prefork first,
though.)


   First, this patch moves all bounds and sanity checking on
MPM-related configuration directives into the open_logs phase.
That has the advantage that it can all be performed in order,
without swapping any configuration tree nodes.

   It also means that this checking gets performed regardless of
whether the user has defined a particular directive or not.  That
means that item B above gets fixed "automatically".  Item A, for
the worker and event MPMs, will also get fixed in the same manner.
All that's required is to make sure that the default values for
all the directives are set properly in pre_config.  Then the
handlers for whatever directives are defined in the config files
run and change those values, and finally open_logs does all the
in-order bounds and sanity checking.


   Second, I studied the logic surrounding the changed_limit_at_restart
flag and whether it was still necessary.  Here I should perhaps
pause to quickly describe the four different conditions under which
these configuration handlers run (for the prefork, worker, and event
MPMs; winnt is a little different, I believe):

1) In the initial startup process, when stderr prints to the console.
2) In the main loop, after detaching from the startup process;
   stderr has been pointed to /dev/null in apr_proc_detach() in
   the pre_config phase.
3) In a separate process that signals the main running process to
   stop or restart.  Like condition 1, stderr prints to the console.
   The open_logs and post_config phases never run here because
   ap_signal_server() detects the running process, signals it, and exits.
4) In the main loop, after ap_mpm_run() returns because it's caught
   a restart signal, and the running process then re-reads the
   configuration files.

   The comment in set_server_limit() that "the error log is a bit bucket
at this point" actually refers to condition 4.  The issue at hand
is whether the running server, having detected a change to ServerLimit
or ThreadLimit, can report that in the error log or not.

   Going back to the r92530 when the changed_limit_at_restart flag
was introduced and compiling a vintage httpd, it was definitely true
at that time that the error log was not open in condition 4.  However,
this turns out to have been because of a bug in main() which passed
the pconf memory pool to ap_open_logs(), not the plog one.  Because
pconf is cleared at the top of the main loop, the callback registered
by the apr_file_dup() on stderr (to point it to the error log) caused
the file descriptors to be closed while the configuration files
were processed.

   As of r92769, plog is used instead, and that means the error log
is actually open and writable in condition 4.  That's why the
various "WARNING:" messages in the configuration directive handlers,
like set_server_limit(), now turn up in the error log upon restart.

   So, it turns out that the changed_limit_at_restart flag can be
removed, and the warning messages moved from ap_mpm_run() into
open_logs along with all the others.


   Third, this patch attempts to format the messages generated
in open_logs for both conditions 1 and 4.  That is, in condition 1,
they are sent to stderr with the APLOG_STARTUP flag, so that no
timestamp is prepended to them.  They are also a little more
user-friendly and I've retained the "console" formatting, where
lines aren't longer than about 72 characters and the second and
subsequent lines of a message are indented by one space.

   In condition 4, terser one-line messages are printed to the
error log, without APLOG_STARTUP, so that they get the normal
timestamp and message level marker prepended to them.


   Fourth, I added a bounds check to ensure that StartServers
is greater than zero.  It looks to me as though if this is set
to, say, a negative number, then ServerLimit child processes
are forked by startup_children().  That's not horrible, but not
ideal.

   Other bounds and sanity checks could be added: for example,
StartServers <= ServerLimit, MinSpareServers < MaxSpareServers,
MaxSpareServers <= ServerLimit, etc.  These are already checked
elsewhere in the code, but it might be nice to localize them
in one place.  Opinions?


   So ... thoughts, comments, flames?  As I noted, I've love to
get approval on this so I can commit it to trunk, along with matching
changes to worker, event, and winnt.  (I might need a little help
with winnt, though.  :-)  Thanks in advance!

Chris.

=====================================
--- server/mpm/prefork/prefork.c.orig	2006-07-17 13:18:51.714038000 -0400
+++ server/mpm/prefork/prefork.c	2006-07-20 18:03:23.212187000 -0400
@@ -96,9 +96,8 @@
 static int ap_daemons_min_free=0;
 static int ap_daemons_max_free=0;
 static int ap_daemons_limit=0;      /* MaxClients */
-static int server_limit = DEFAULT_SERVER_LIMIT;
+static int server_limit = 0;
 static int first_server_limit = 0;
-static int changed_limit_at_restart;
 static int mpm_state = AP_MPMQ_STARTING;
 static ap_pod_t *pod;
 
@@ -900,14 +899,6 @@
 
     ap_log_pid(pconf, ap_pid_fname);
 
-    first_server_limit = server_limit;
-    if (changed_limit_at_restart) {
-        ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
-                     "WARNING: Attempt to change ServerLimit "
-                     "ignored during restart");
-        changed_limit_at_restart = 0;
-    }
-
     /* Initialize cross-process accept lock */
     ap_lock_fname = apr_psprintf(_pconf, "%s.%" APR_PID_T_FMT,
                                  ap_server_root_relative(_pconf, ap_lock_fname),
@@ -1245,22 +1236,140 @@
  */
 static int prefork_open_logs(apr_pool_t *p, apr_pool_t *plog, apr_pool_t *ptemp, server_rec
*s)
 {
+    static int restart_num = 0;
+    int startup = 0;
+    int level_flags = 0;
     apr_status_t rv;
 
     pconf = p;
     ap_server_conf = s;
 
+    /* the reverse of pre_config, we want this only the first time around */
+    if (restart_num++ == 0) {
+        startup = 1;
+        level_flags |= APLOG_STARTUP;
+    }
+
     if ((num_listensocks = ap_setup_listeners(ap_server_conf)) < 1) {
-        ap_log_error(APLOG_MARK, APLOG_ALERT|APLOG_STARTUP, 0,
-                     NULL, "no listening sockets available, shutting down");
+        ap_log_error(APLOG_MARK, APLOG_ALERT|level_flags, 0,
+                     (startup ? NULL : s),
+                     "no listening sockets available, shutting down");
         return DONE;
     }
 
     if ((rv = ap_mpm_pod_open(pconf, &pod))) {
-        ap_log_error(APLOG_MARK, APLOG_CRIT|APLOG_STARTUP, rv, NULL,
-                "Could not open pipe-of-death.");
+        ap_log_error(APLOG_MARK, APLOG_CRIT|level_flags, rv,
+                     (startup ? NULL : s),
+                     "could not open pipe-of-death");
         return DONE;
     }
+
+    if (ap_daemons_to_start < 1) {
+        if (startup) {
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         "WARNING: StartServers of %d not allowed, "
+                         "increasing to 1.", ap_daemons_to_start);
+        } else {
+            ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
+                         "StartServers of %d not allowed, increasing to 1",
+                         ap_daemons_to_start);
+        }
+        ap_daemons_to_start = 1;
+    }
+
+    if (ap_daemons_min_free < 1) {
+        if (startup) {
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         "WARNING: MinSpareServers of %d not allowed, "
+                         "increasing to 1", ap_daemons_min_free);
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         " to avoid almost certain server failure.");
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         " Please read the documentation.");
+        } else {
+            ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
+                         "MinSpareServers of %d not allowed, increasing to 1",
+                         ap_daemons_min_free);
+        }
+        ap_daemons_min_free = 1;
+    }
+
+    if (server_limit > MAX_SERVER_LIMIT) {
+        if (startup) {
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         "WARNING: ServerLimit of %d exceeds compile-time "
+                         "limit of", server_limit);
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         " %d servers, lowering to %d.",
+                         MAX_SERVER_LIMIT, MAX_SERVER_LIMIT);
+        } else {
+            ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
+                         "ServerLimit of %d exceeds compile-time limit "
+                         "of %d, lowering to match",
+                         server_limit, MAX_SERVER_LIMIT);
+        }
+        server_limit = MAX_SERVER_LIMIT;
+    }
+    else if (server_limit < 1) {
+        if (startup) {
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         "WARNING: ServerLimit of %d not allowed, "
+                         "increasing to 1.", server_limit);
+        } else {
+            ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
+                         "ServerLimit of %d not allowed, increasing to 1",
+                         server_limit);
+        }
+        server_limit = 1;
+    }
+
+    /* you cannot change ServerLimit across a restart; ignore
+     * any such attempts
+     */
+    if (!first_server_limit) {
+        first_server_limit = server_limit;
+    }
+    else if (server_limit != first_server_limit) {
+        /* don't need a startup console version here */
+        ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
+                     "changing ServerLimit to %d from original value of %d "
+                     "not allowed during restart",
+                     server_limit, first_server_limit);
+        server_limit = first_server_limit;
+    }
+
+    if (ap_daemons_limit > server_limit) {
+        if (startup) {
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         "WARNING: MaxClients of %d exceeds ServerLimit "
+                         "value of", ap_daemons_limit);
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         " %d servers, lowering MaxClients to %d.",
+                         server_limit, server_limit);
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         " To increase, please see the ServerLimit "
+                         "directive.");
+        } else {
+            ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
+                         "MaxClients of %d exceeds ServerLimit value "
+                         "of %d, lowering to match",
+                         ap_daemons_limit, server_limit);
+        }
+        ap_daemons_limit = server_limit;
+    }
+    else if (ap_daemons_limit < 1) {
+        if (startup) {
+            ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_STARTUP, 0, NULL,
+                         "WARNING: MaxClients of %d not allowed, "
+                         "increasing to 1.", ap_daemons_limit);
+        } else {
+            ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s,
+                         "MaxClients of %d not allowed, increasing to 1",
+                         ap_daemons_limit);
+        }
+        ap_daemons_limit = 1;
+    }
+
     return OK;
 }
 
@@ -1307,6 +1416,7 @@
     ap_daemons_to_start = DEFAULT_START_DAEMON;
     ap_daemons_min_free = DEFAULT_MIN_FREE_DAEMON;
     ap_daemons_max_free = DEFAULT_MAX_FREE_DAEMON;
+    server_limit = DEFAULT_SERVER_LIMIT;
     ap_daemons_limit = server_limit;
     ap_pid_fname = DEFAULT_PIDLOG;
     ap_lock_fname = DEFAULT_LOCKFILE;
@@ -1359,16 +1469,6 @@
     }
 
     ap_daemons_min_free = atoi(arg);
-    if (ap_daemons_min_free <= 0) {
-       ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-                    "WARNING: detected MinSpareServers set to non-positive.");
-       ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-                    "Resetting to 1 to avoid almost certain Apache failure.");
-       ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-                    "Please read the documentation.");
-       ap_daemons_min_free = 1;
-    }
-
     return NULL;
 }
 
@@ -1391,62 +1491,17 @@
     }
 
     ap_daemons_limit = atoi(arg);
-    if (ap_daemons_limit > server_limit) {
-       ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-                    "WARNING: MaxClients of %d exceeds ServerLimit value "
-                    "of %d servers,", ap_daemons_limit, server_limit);
-       ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-                    " lowering MaxClients to %d.  To increase, please "
-                    "see the ServerLimit", server_limit);
-       ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-                    " directive.");
-       ap_daemons_limit = server_limit;
-    }
-    else if (ap_daemons_limit < 1) {
-        ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-                     "WARNING: Require MaxClients > 0, setting to 1");
-        ap_daemons_limit = 1;
-    }
     return NULL;
 }
 
 static const char *set_server_limit (cmd_parms *cmd, void *dummy, const char *arg)
 {
-    int tmp_server_limit;
-
     const char *err = ap_check_cmd_context(cmd, GLOBAL_ONLY);
     if (err != NULL) {
         return err;
     }
 
-    tmp_server_limit = atoi(arg);
-    /* you cannot change ServerLimit across a restart; ignore
-     * any such attempts
-     */
-    if (first_server_limit &&
-        tmp_server_limit != server_limit) {
-        /* how do we log a message?  the error log is a bit bucket at this
-         * point; we'll just have to set a flag so that ap_mpm_run()
-         * logs a warning later
-         */
-        changed_limit_at_restart = 1;
-        return NULL;
-    }
-    server_limit = tmp_server_limit;
-
-    if (server_limit > MAX_SERVER_LIMIT) {
-       ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-                    "WARNING: ServerLimit of %d exceeds compile time limit "
-                    "of %d servers,", server_limit, MAX_SERVER_LIMIT);
-       ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-                    " lowering ServerLimit to %d.", MAX_SERVER_LIMIT);
-       server_limit = MAX_SERVER_LIMIT;
-    }
-    else if (server_limit < 1) {
-        ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-                     "WARNING: Require ServerLimit > 0, setting to 1");
-        server_limit = 1;
-    }
+    server_limit = atoi(arg);
     return NULL;
 }
 
=====================================

Mime
View raw message