crowell / modpagespeed_tmp

Automatically exported from code.google.com/p/modpagespeed
Apache License 2.0
0 stars 0 forks source link

apache should fail to restart with a clean message if memcached fails to initialize #560

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Currently the module calls abort() if it notices a problem connecting to 
memcached.  This doesn't provide an actionable error message to the user.

Note from modules-dev@ on how to do this:

From: sorinm@gmail.com, Oct 22:
...you can exit nicely in post_config, which is run (as root) after the conf is 
read but before the children are spawned. It suffices to return a non-ok code 
for apache to exit. I _think_, I'm not sure, that what you log in post_config 
does not go on the console but goes to the server log. But maybe there's a way 
to put the error message on the console too.

From: trawick@gmail.com, Oct 22:
exit(APEXIT_CHILDFATAL) is better than abort(), but if at all possible
you should handle this in the parent

where the output goes depends on the phase of initialization

1st time post-config is called: ap_log_error goes nowhere
subsequent times post-config is called: ap_log_error goes to the error log

Some other modules handle this issue with an open_logs hook that runs
before core's open_logs hook.  The module can then log an error that
goes to the console and return an error so that startup is aborted.

Example from server/mpm/prefork/prefork.c:

static void prefork_hooks(apr_pool_t *p)
{
    /* The prefork open_logs phase must run before the core's, or stderr
     * will be redirected to a file, and the messages won't print to the
     * console.
     */
    static const char *const aszSucc[] = {"core.c", NULL};
...
    ap_hook_open_logs(prefork_open_logs, NULL, aszSucc, APR_HOOK_MIDDLE);

Original issue reported on code.google.com by jmara...@google.com on 9 Nov 2012 at 1:39

GoogleCodeExporter commented 9 years ago
Upon further inspection, I don't believe the abort() call in our code can be 
reached even if memcached is frozen or not running.  AprMemCache::Connect() 
does not actually do socket operations.  So Apache can start up, mod_pagespeed 
will notice that cache results keep timing out or failing, and give up trying 
to cache anything or do rewrites for 30 seconds, then try again.

This may be fine.  We can remove the abort() call and it won't make any 
difference.

One thing to consider is whether we should refuse to allow Apache to start if 
memcached is dead or unreachable.  I'm currently leaning against that idea 
because mod_pagespeed's failure mode in this case is to skip optimizing 
resources and try again in 30 seconds.

Original comment by jmara...@google.com on 9 Nov 2012 at 3:35

GoogleCodeExporter commented 9 years ago
I was not able to actually get Apache to fail to initialize, even starting 
Apache with memcached down.  This is because in our startup flow we were not 
actually doing any transactions with memcached.  So we know how to fix this 
problem as shown in the bug description, but we currently don't actually reach 
the abort().

We could consider doing a GetStatus to validate the connection.  The problem is 
that would prevent Apache from starting if memcached wasn't already running, 
and that seems like bad server karma.  The current behavior is that Apache will 
start, mod_pagespeed will quickly notice that memcached is hung, and will 
abstain from optimizing resources for 30 seconds and then try again.  That 
seems like the right behavior.

Original comment by jmara...@google.com on 30 Nov 2012 at 2:55