Fix clock skew tolerance in deleteAll; make it configurable [Follow-up: Add mechanism to support invalidateAll and deleteAll]

To comprehend this a little bit:

  /**
   * Marks the fast cache bin as outdated because of a write.
   */
  protected function markAsOutdated() {

    // Clocks on a single server can drift. Multiple servers may have slightly
    // differing opinions about the current time. Given that, do not assume
    // 'now' on this server is always later than our stored timestamp.
    // Also add 1 millisecond, to ensure that caches written earlier in the same
    // millisecond are invalidated. It is possible that caches will be later in
    // the same millisecond and are then incorrectly invalidated, but that only
    // costs one additional roundtrip to the persistent cache.
    $now = round(microtime(TRUE) + 0.001, 3);
    if ($now > $this
      ->getLastWriteTimestamp()) {
      $this->lastWriteTimestamp = $now;
      $this->consistentBackend
        ->set(self::LAST_WRITE_TIMESTAMP_PREFIX . $this->bin, $this->lastWriteTimestamp);
    }
  }

We really should be using this approach when writing the timestamp + configurable skew for the decision of safe vs. fast.

This does another thing (and yes it is correct):

If someone else has already written a later timestamp we _never_ write an earlier timestamp.

which is super important to fix clock skew in both directions.

Which is another reason why I think we should have a timestamp service in memcache and later in core, which we inject so that clock skew tolerance can be taken into account without having to re-implement the wheel 10x.

Cause now it seems both cache tags and deleteAll() need to implement the exact same code.

And in a way thats true, a cache tag is just one invalidation timestamp, so sure it has the same properties as the deleteAll deletion timestamp for the bin ...

Here is a first patch based on the new invalidator service:

diff --git a/src/MemcacheBackend.php b/src/MemcacheBackend.php
index a034421..fba61f7 100644
--- a/src/MemcacheBackend.php
+++ b/src/MemcacheBackend.php
@@ -75,12 +75,13 @@ class MemcacheBackend implements CacheBackendInterface {
    * @param \Drupal\Core\Cache\CacheTagsChecksumInterface $checksum_provider
    *   The cache tags checksum service.
    */
-  public function __construct($bin, DrupalMemcacheInterface $memcache, LockBackendInterface $lock, DrupalMemcacheConfig $settings, CacheTagsChecksumInterface $checksum_provider) {
+  public function __construct($bin, DrupalMemcacheInterface $memcache, LockBackendInterface $lock, DrupalMemcacheConfig $settings, CacheTagsChecksumInterface $checksum_provider, TimestampInvalidatorInterface $timestamp_invalidator) {
     $this->bin = $bin;
     $this->memcache = $memcache;
     $this->lock = $lock;
     $this->settings = $settings;
     $this->checksumProvider = $checksum_provider;
+    $this->timestampInvalidator = $timestamp_invalidator;
 
     $this->ensureBinDeletionTimeIsSet();
   }
@@ -247,14 +248,14 @@ class MemcacheBackend implements CacheBackendInterface {
    * {@inheritdoc}
    */
   public function deleteAll() {
-    $this->updateBinLastDeletionTime();
+    $this->lastBinDeletionTime = $this->timestampInvalidator->invalidateTimestamp($this->bin);
   }
 
   /**
    * {@inheritdoc}
    */
   public function invalidate($cid) {
-    $this->invalidateMultiple((array) $cid);
+    $this->invalidateMultiple([$cid]);
   }
 
   /**
@@ -298,7 +299,7 @@ class MemcacheBackend implements CacheBackendInterface {
    * {@inheritdoc}
    */
   public function removeBin() {
-    $this->updateBinLastDeletionTime();
+    $this->lastBinDeletionTime = $this->timestampInvalidator->invalidateTimestamp($this->bin);
   }
 
   /**
@@ -344,16 +345,8 @@ class MemcacheBackend implements CacheBackendInterface {
     // to compare with.
     if (!$last_bin_deletion) {
       return FALSE;
-    }
 
-    // Clocks on a single server can drift. Multiple servers may have slightly
-    // differing opinions about the current time. Given that, do not assume
-    // 'now' on this server is always later than our stored timestamp.
-    // Also add 1 millisecond, to ensure that caches written earlier in the same
-    // millisecond are invalidated. It is possible that caches will be later in
-    // the same millisecond and are then incorrectly invalidated, but that only
-    // costs one additional roundtrip to the persistent cache.
-    return (($item_microtime + .001) > $last_bin_deletion);
+    return $item_microtime > $last_bin_deletion;
   }
 
   /**
@@ -365,49 +358,21 @@ class MemcacheBackend implements CacheBackendInterface {
    */
   protected function getBinLastDeletionTime() {
     if (!isset($this->lastBinDeletionTime)) {
-      $this->lastBinDeletionTime = $this->memcache->get($this->getBinLastDeletionTimeKey());
+      $this->lastBinDeletionTime = $this->timestampInvalidator->getLastInvalidationTimestamp($this->bin);
     }
 
     return $this->lastBinDeletionTime;
   }
 
   /**
-   * Updates the last invalidation time for the bin.
-   *
-   * @internal
-   *
-   * @return bool|\Drupal\memcache\DrupalMemcacheInterface
-   */
-  protected function updateBinLastDeletionTime() {
-    $now = round(microtime(TRUE), 3);
-
-    $return = $this->memcache->set($this->getBinLastDeletionTimeKey(), $now);
-
-    $this->lastBinDeletionTime = $now;
-
-    return $return;
-  }
-
-  /**
    * Ensures a last bin deletion time has been set.
    *
    * @internal
    */
   protected function ensureBinDeletionTimeIsSet() {
     if (!$this->getBinLastDeletionTime()) {
-      $this->updateBinLastDeletionTime();
+      $this->lastBinDeletionTime = $this->timestampInvalidator->invalidateTimestamp($this->bin);
     }
   }
 
-  /**
-   * Gets the invalidation time cache key.
-   *
-   * @internal
-   *
-   * @return string
-   */
-  protected function getBinLastDeletionTimeKey() {
-    return sprintf('bin_deletion:%s', $this->bin);
-  }
-
 }

Just a question, but otherwise RTBC - let's get this in!

+++ b/src/MemcacheBackend.php
@@ -36,24 +38,34 @@ class MemcacheBackend implements CacheBackendInterface {
-   * @var \Drupal\Core\Cache\CacheTagsChecksumInterface
+   * @var CacheTagsChecksumInterface|CacheTagsInvalidatorInterface
...
-   * @param \Drupal\Core\Cache\CacheTagsChecksumInterface $checksum_provider
+   * @param CacheTagsChecksumInterface $checksum_provider

Are IDE's now namespace aware? Before it was needed to put the full class for IDE's to find the classes.