MemcacheLockBackend service should use "add" and "delete" instead of "get" and "set"

Tagging as a beta blocker, per #2989594: [META] Create a stable release of Memcache module for Drupal 8.

Would this cause the problem with Acquia production instances failing, because they're connecting to multiple memcache instances?

I agree, this is just plain wrong:

D7 code for reference:

function lock_acquire($name, $timeout = 30) {

// [...]

  if (dmemcache_add($name, _lock_id(), $timeout, 'semaphore')) {
    $locks[$name] = _lock_id();
  }
  elseif (($result = dmemcache_get($name, 'semaphore')) && isset($locks[$name]) && $locks[$name] == $result) {
    // Only renew the lock if we already set it and it has not expired.
    dmemcache_set($name, _lock_id(), $timeout, 'semaphore');
  }
  else {
    // Failed to acquire the lock.  Unset the key from the $locks array even if
    // not set, PHP 5+ allows this without error or warning.
    unset($locks[$name]);
  }

  return isset($locks[$name]);
}

Marking as critical as a non-atomic lock implementation is a recipe for disaster.

Note from code review:

- PersistentMemcacheLockBackend will have a lot of trouble to be correct

Memcache is non-volatile storage and it can evict any key at any time, which does not combine with potentially long-lived locks (as persistent seems to imply:

I suggest we do defer to the database for that and do not implement it. If it is for across page requests, then it should be fine to live in the database as that should mostly be used for admin things, etc.

The problem is:

If the key or keys are not there, we cannot assume that the lock is not taken, but we also cannot assume that the lock is taken. We are in an unknown state.

For normal locks the same problem exists, but as they are way more short-lived (usually just to rebuild some data / caches), the chances of eviction are way way less. Indeed so less, that we've not seen any such problems on sites in years.

The persistent lock could in theory be made more safe by putting different sized items into the different memcache slabs (after we have the lock) and do a check if any of those items still exist before acquiring the lock - to detect the evicted lock.

That could in theory make this work - will speak with Narayan again about it today - if putting in several slabs would make us more safe here.

I also suggest we open a follow-up for the persistent lock discussion ;).

My fault for the initial (very quick) implementation many years ago :)

Here is a patch to us add() (wasn't previously a part of our abstracted memcache wrapper API).

Agree, the persistent lock is probably not a good idea. Might be an idea to consider removing it. It was mainly copied from core lock previously...

1. +++ b/src/DrupalMemcache.php
   @@ -84,6 +84,22 @@ class DrupalMemcache extends DrupalMemcacheBase {
   +    $collect_stats = $this->stats_init();
   

   That's unrelated, but why not put the collectStats in $this in __construct?

   Also I think stat collection should be a decorator around the class, not hard-coded as we have the possibility to do so here.

   Can we open a ticket for that?

2. +++ b/src/Lock/MemcacheLockBackend.php
   @@ -61,7 +64,7 @@ class MemcacheLockBackend extends LockBackendAbstract {
   -      $success = !$this->lockMayBeAvailable($name) && $this->memcache->set($key, $lock_id, $timeout);
   +      $success = !$this->lockMayBeAvailable($name) && $this->memcache->add($key, $lock_id, $timeout);
   

   Not really, we can only extend with set ;), that's the wrong line to change.

3. +++ b/src/Lock/MemcacheLockBackend.php
   @@ -127,9 +130,13 @@ class MemcacheLockBackend extends LockBackendAbstract {
   -    return 'lock:' . $this->bin . ':' . $name;
   +    return sprintf('lock:%s:%s', $this->bin, $name);
   

   Not opposed, but: Why this change?

Ugh - yes.

1. Yes, let's open a ticket for it to discuss
2. Fixed!
3. Just because it's easier to read, sneaking in....

Added #2995907: Remove Persistent Lock Implementation for the removal of the persistent lock

RTBC - lets get this in

Follow-up for statistics collection as decorator here: #2996054: Refactor statistics collection to a decorator agreed that'd be a good change to make.