Better logging and handling of connect errors

See http://drupalcode.org/project/memcache.git/blob/refs/heads/7.x-1.x:/dmem...

Thanks for the link. Good to know about the status update:

// We can't use watchdog because this happens in a bootstrap phase
// where watchdog is non-functional. Thus use trigger_error() to
// start drupal_error_handler().

Is there another way to to make this more obvious to people?

Is it better to just add a $conf variable to hide this? Since watchdog() is unavailable, I'm not sure how to make this message be manageable outside of simply being displayed.

I've attached a patch which adds a variable check before printing the error. I decided to default the new variable, "display_memcache_connection_error," to TRUE in order to not change the behavior of this module. In my personal opinion, I would default this to FALSE, but I wanted to avoid changing how the module currently functions.

This patch allows you to define $conf['display_memcache_connection_error'] = FALSE; in your settings.php (or drush vset it) in order to not display a memcache connection error.

If we're strongly encouraging this setting and there were multiple "bug report" issues describing this, I think we could probably disable this by default. I can't imagine any situation where you would want this to display on production environments. I think we should just default it to FALSE.

Another thought - With watchdog not available yet, maybe we should consider a static variable that will produce a proper watchdog message once that portion of the bootstrap is ready? Have a parallel variable to $failed_connection_cache[$s] that will output a watchdog message for each server. Or with this variable, we could even wait until user_access() is available and output an appropriate message there.

You had a typo in "varaible_get", so here's a corrected patch too.

Can I suggest using the register_shutdown_function technique as used elsewhere in dmemcache_object().

The error will end up in watchdog, so is more likely to be seen than an error message which is disabled by default- what do you think?

We would have to check if watchdog ever loaded, because it could be a failed bootstrap theoretically. Other than that, I like this approach much better.

If watchdog doesn't exist it will either fail silently or throw the following warning:

Warning: register_shutdown_function(): Invalid shutdown callback 'watchdog' passed in dmemcache_object()

Do you think checking for watchdog is necessary in this case?

We are looking to distribute memcache across several caching nodes and were surprised to see this error message being seemingly dumped to end users when a node is removed. Our hope was that a single or even a couple nodes could die (either expectedly for patching or unexpectedly) and there would be minimal impact to end users.

As memcache is billed as a distributed caching system it seems odd to me that this module would throw an error message to the screen when a node dies. I mean, as you add nodes wouldn't that invariably increase the likelyhood that any single point could fail?

Am I missing something here (like this module does not work well with distributed sources) or is this just a really problematic bug?

@markpavlitski - Can we just write a wrapper for the shutdown function that checks if watchdog is available, then log if it is?

@gp177 - which error message is being displayed to your users? And do you have error reporting set to None?

@erikwebb - watchdog() is defined in bootstrap.inc, so should always be available. It's not guaranteed that any modules will be loaded which implement hook_watchdog() yet, but watchdog already checks for that and fails gracefully.

@markpavlitski - Oh okay, so theoretically we'll never see that error? Nevermind then. I'll test out the patch ASAP and mark RTBC.

@erikwebb - yea, that's the error you get if you register a non-existent shutdown function, but having looked at it more closely you wouldn't ever get it with watchdog.

@erikwebb - Did you get a chance to test the patch in #6? Would be good to see a few issues marked RTBC.

Sorry it's taken so long to get this RTBC'd. The patch in #6 works beautifully.

I've tested on a stack with two memcache servers. Turning one or both memcache servers off no-longer displays errors to the screen but it does successfully log to watchdog. Big win!

Nice improvement @markpavlitski, committed:
http://drupalcode.org/project/memcache.git/commit/dab8e89

Tested #6 - works for me.

Thank you.

Does this create a dependency on Watchdog? What if you are not using watchdog?

Added checking to #6 in the event dblog is not enabled.

I think ElusiveMind has a point. If you turn off the dblog module you are going to run in to errors.

I've tested this with and without the dblog module enabled, and not run into any errors. Also note that module_exists() will not always be available at this point.

what about using function_exists to test for the call before we run this?

Jeremy,

You are correct. I forgot the watchdog function is in core, not the dblog module. :/

PW