Enhance failure tolerance

Your patch certainly reduced the number of warnings I get on screen if a particular bin (cache_menu's in my case) was killed. I still get a couple of them though:

Notice: Memcache::connect() [memcache.connect]: Server 127.0.0.1 (tcp 11212) failed with: Connection refused (111) in dmemcache_object() (line 293 of /var/www/html/drupal/sites/all/modules/contrib/memcache/dmemcache.inc).

In my case, line 293 is marked in the code snippet below:

foreach ($memcache_servers as $s => $c) {
        if ($c == $cluster && (!is_array($failed_connection_cache) || !array_key_exists($s, $failed_connection_cache))) {
          list($host, $port) = explode(':', $s);

          // This is a server that belongs to this cluster.
          if (!class_exists('Memcached') && !$init) {
            // If using PECL memcache extension, use ->connect for first server
            if ($memcache->connect($host, $port)) { /*** LINE 293 ***/
              $init = TRUE;
            }
          }
          else {
            if ($memcache->addServer($host, $port) && !$init) {
              $init = TRUE;
            }
          }
          if (!$init) {
            // @TODO Find a way to create a logentry. This is to early in
            // bootstrap to use watchdog.
            $failed_connection_cache[$s] = FALSE;
          }
        }
      }

--- dmemcache.inc
+++ dmemcache.inc
@@ -93,6 +93,7 @@ function dmemcache_add($key, $value, $exp = 0, $bin = 'cache', $mc = NULL, $flag
  */
 function dmemcache_get($key, $bin = 'cache', $mc = NULL) {
   global $_memcache_statistics;
+  $result = NULL;
   $full_key = dmemcache_key($key, $bin);
   $statistics = array('get', $bin, $full_key);
   $success = '0';

This is wrong. The function documentation says that it returns FALSE on error, which is different from NULL.

See correct patch at #1307078: Fix Notice: Undefined variable: result in dmemcache_get() (line 114 of /var/www/sites/all/modules/memcache/dmemcache.inc).

Closed as duplicate: #1307078: Fix Notice: Undefined variable: result in dmemcache_get() (line 114 of /var/www/sites/all/modules/memcache/dmemcache.inc).

Suggestions:

Change:

+        if ($c == $cluster && (!is_array($failed_connection_cache) || !array_key_exists($s, $failed_connection_cache))) {

To:

+        if ($c == $cluster && !isset($failed_connection_cache[$s])) {

Change:

+        if (!$init) {
+          // @TODO Find a way to create a logentry. This is to early in
+          // bootstrap to use watchdog.
+          $failed_connection_cache[$s] = FALSE;
+        }

To:

+        if (!$init) {
+          error_log(st('Failed to connect to memcache server: %server', array('@server' => $s)));
+          $failed_connection_cache[$s] = FALSE;
+        }

The functions isset() and empty() do not work the same way.

A variable set to FALSE will be both set and empty.

A variable set to NULL will be both unset and empty.

The following should output true.

php -r '$x = FALSE; var_export(empty($x));'

So should this:

php -r '$x = FALSE; var_export(isset($x));'

And this:

php -r '$x = NULL; var_export(empty($x));'

But this will output false:

php -r '$x = NULL; var_export(isset($x));'

Patch in #6 looks good to me. Using in production now.

Found one mistake, which was my own fault:

+          error_log(st('Failed to connect to memcache server: %server', array('@server' => $s)));

Should be:

+          error_log(st('Failed to connect to memcache server: %server', array('%server' => $s)));

(sorry)

Good to go.

Subscribe, will try to review and test this asap.

+++ b/dmemcache.incundefined
@@ -393,6 +394,10 @@ function dmemcache_object($bin = NULL, $flush = FALSE) {
+        if (!$init) {
+          error_log(st('Failed to connect to memcache server: %server', array('%server' => $s)));
+          $failed_connection_cache[$s] = TRUE;

Is there a specific reason to not use watchdog() here? That is imho the correct function to use for messages like this, unless there is a reason why it can't be used.

+++ b/memcache.incundefined
@@ -146,6 +146,10 @@ class MemCacheDrupal implements DrupalCacheInterface {
+    if ($this->memcache === FALSE) {
+      // No memcache connection.
+      return;

If this happens, we should have already logged a message at the time the connection failed, right?

Is there a specific reason to not use watchdog() here? That is imho the correct function to use for messages like this, unless there is a reason why it can't be used.

Yes. This code is invoked during bootstrap, before watchdog is available. So we have three choices:

1. Fail silently.

2. Fail while gracefully reporting an error using the only means available.

3. Deliberately invoke watchdog() to produce an ungraceful fatal error.

If this happens, we should have already logged a message at the time the connection failed, right?

Precisely. No need to log the same error multiple times.

I see. Looking at watchdog(), which is defined in bootstrap.inc and therefore always available, it is basically a no-op if module_implements() is not defined. Problem is, in that case, the message won't be logged at all. Not sure about trigger_error() either, not sure what happens if e.g. a E_USER_WARNING is triggered during bootstrap.

So error_log() might very well be the only option here, but maybe you could add a quick comment explaining why it's used instead of something else?

trigger_error() is good, but error_log() should be removed.

Do we really need to cache the fact that connections failed?

+// Check if memcached is available - if not switch back to default lock handler.
+if (!dmemcache_object('semaphore')) {
+  require_once DRUPAL_ROOT . '/includes/lock.inc';
 }
+else {
 

I'm not keen at all on doing this, conditionally defining functions is not pretty although I don't see a way around that without backporting #1225404: Modern event based lock framework proposal to D7 (which I would like to do though if it gets in).

I'd be happier with the lock.inc change if that was moved to an include maybe. At least then there's not weird indentation if the function declarations.

Looks much better - there's at least one comment over 80 chars though, also @TODO should be @todo.

+// @TODO get rid of this conditional include as soon as this is done: http://drupal.org/node/1225404

OK this looks fine now. Committed/pushed to 7.x-1.x, we could probably backport this to 6.x

Back ported:

• http://cgit.drupalcode.org/memcache/commit/?id=5502a40
• http://cgit.drupalcode.org/memcache/commit/?id=539594d