bug #33959 [Cache] fix 2RTT + race condition in AbstractTagAwareAdapt…

…er::deleteItems() (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Cache] fix 2RTT + race condition in AbstractTagAwareAdapter::deleteItems()

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

A final improvement to `AbstractTagAwareAdapter::deleteItems()`: this PR makes `deleteItems()` operate in 1RTT instead of 2.

Commits
-------

0613c22 [Cache] fix 2RTT + race condition in AbstractTagAwareAdapter::deleteItems()

src/Symfony/Component/Cache/Adapter/AbstractTagAwareAdapter.php

@@ -133,12 +133,18 @@ abstract protected function doSave(array $values, ?int $lifetime, array $addTagD
     /**
      * Removes multiple items from the pool and their corresponding tags.
      *
-     * @param array $ids     An array of identifiers that should be removed from the pool
-     * @param array $tagData Optional array of tag identifiers => key identifiers that should be removed from the pool
+     * @param array $ids An array of identifiers that should be removed from the pool
      *
      * @return bool True if the items were successfully removed, false otherwise
      */
-    abstract protected function doDelete(array $ids, array $tagData = []): bool;
+    abstract protected function doDelete(array $ids);
+
+    /**
+     * Removes relations between tags and deleted items.
+     *
+     * @param array $tagData Array of tag => key identifiers that should be removed from the pool
+     */
+    abstract protected function doDeleteTagRelations(array $tagData): bool;
 
     /**
      * Invalidates cached items using tags.
@@ -150,21 +156,21 @@ abstract protected function doDelete(array $ids, array $tagData = []): bool;
     abstract protected function doInvalidate(array $tagIds): bool;
 
     /**
-     * Returns the tags bound to the provided ids.
+     * Delete items and yields the tags they were bound to.
      */
-    protected function doFetchTags(array $ids): iterable
+    protected function doDeleteYieldTags(array $ids): iterable
     {
         foreach ($this->doFetch($ids) as $id => $value) {
             yield $id => \is_array($value) && \is_array($value['tags'] ?? null) ? $value['tags'] : [];
         }
+
+        $this->doDelete($ids);
     }
 
     /**
      * {@inheritdoc}
-     *
-     * @return bool
      */
-    public function commit()
+    public function commit(): bool
     {
         $ok = true;
         $byLifetime = $this->mergeByLifetime;
@@ -223,17 +229,14 @@ public function commit()
 
     /**
      * {@inheritdoc}
-     *
-     * Overloaded in order to deal with tags for adjusted doDelete() signature.
-     *
-     * @return bool
      */
-    public function deleteItems(array $keys)
+    public function deleteItems(array $keys): bool
     {
         if (!$keys) {
             return true;
         }
 
+        $ok = true;
         $ids = [];
         $tagData = [];
 
@@ -243,24 +246,22 @@ public function deleteItems(array $keys)
         }
 
         try {
-            foreach ($this->doFetchTags($ids) as $id => $tags) {
+            foreach ($this->doDeleteYieldTags(array_values($ids)) as $id => $tags) {
                 foreach ($tags as $tag) {
                     $tagData[$this->getId(self::TAGS_PREFIX.$tag)][] = $id;
                 }
             }
         } catch (\Exception $e) {
-            // ignore unserialization failures
+            $ok = false;
         }
 
         try {
-            if ($this->doDelete(array_values($ids), $tagData)) {
+            if ((!$tagData || $this->doDeleteTagRelations($tagData)) && $ok) {
                 return true;
             }
         } catch (\Exception $e) {
         }
 
-        $ok = true;
-
         // When bulk-delete failed, retry each item individually
         foreach ($ids as $key => $id) {
             try {

src/Symfony/Component/Cache/Adapter/FilesystemTagAwareAdapter.php

@@ -27,7 +27,6 @@ class FilesystemTagAwareAdapter extends AbstractTagAwareAdapter implements Prune
     use FilesystemTrait {
         doClear as private doClearCache;
         doSave as private doSaveCache;
-        doDelete as private doDeleteCache;
     }
 
     /**
@@ -133,14 +132,19 @@ protected function doSave(array $values, ?int $lifetime, array $addTagData = [],
     /**
      * {@inheritdoc}
      */
-    protected function doFetchTags(array $ids): iterable
+    protected function doDeleteYieldTags(array $ids): iterable
     {
         foreach ($ids as $id) {
             $file = $this->getFile($id);
             if (!file_exists($file) || !$h = @fopen($file, 'rb')) {
                 continue;
             }
 
+            if ((\PHP_VERSION_ID >= 70300 || '\\' !== \DIRECTORY_SEPARATOR) && !@unlink($file)) {
+                fclose($h);
+                continue;
+            }
+
             $meta = explode("\n", fread($h, 4096), 3)[2] ?? '';
 
             // detect the compact format used in marshall() using magic numbers in the form 9D-..-..-..-..-00-..-..-..-5F
@@ -161,25 +165,26 @@ protected function doFetchTags(array $ids): iterable
             }
 
             fclose($h);
+
+            if (\PHP_VERSION_ID < 70300 && '\\' === \DIRECTORY_SEPARATOR) {
+                @unlink($file);
+            }
         }
     }
 
     /**
      * {@inheritdoc}
      */
-    protected function doDelete(array $ids, array $tagData = []): bool
+    protected function doDeleteTagRelations(array $tagData): bool
     {
-        $ok = $this->doDeleteCache($ids);
-
-        // Remove tags
-        foreach ($tagData as $tagId => $idMap) {
+        foreach ($tagData as $tagId => $idList) {
             $tagFolder = $this->getTagFolder($tagId);
-            foreach ($idMap as $id) {
+            foreach ($idList as $id) {
                 @unlink($this->getFile($id, false, $tagFolder));
             }
         }
 
-        return $ok;
+        return true;
     }
 
     /**

src/Symfony/Component/Cache/Adapter/RedisTagAwareAdapter.php

@@ -123,10 +123,11 @@ protected function doSave(array $values, ?int $lifetime, array $addTagData = [],
     /**
      * {@inheritdoc}
      */
-    protected function doFetchTags(array $ids): iterable
+    protected function doDeleteYieldTags(array $ids): iterable
     {
         $lua = <<<'EOLUA'
             local v = redis.call('GET', KEYS[1])
+            redis.call('DEL', KEYS[1])
 
             if not v or v:len() <= 13 or v:byte(1) ~= 0x9D or v:byte(6) ~= 0 or v:byte(10) ~= 0x5F then
                 return ''
@@ -159,25 +160,12 @@ protected function doFetchTags(array $ids): iterable
     /**
      * {@inheritdoc}
      */
-    protected function doDelete(array $ids, array $tagData = []): bool
+    protected function doDeleteTagRelations(array $tagData): bool
     {
-        if (!$ids) {
-            return true;
-        }
-
-        $predisCluster = $this->redis instanceof \Predis\ClientInterface && $this->redis->getConnection() instanceof PredisCluster;
-        $this->pipeline(static function () use ($ids, $tagData, $predisCluster) {
-            if ($predisCluster) {
-                // Unlike phpredis, Predis does not handle bulk calls for us against cluster
-                foreach ($ids as $id) {
-                    yield 'del' => [$id];
-                }
-            } else {
-                yield 'del' => $ids;
-            }
-
+        $this->pipeline(static function () use ($tagData) {
             foreach ($tagData as $tagId => $idList) {
-                yield 'sRem' => array_merge([$tagId], $idList);
+                array_unshift($idList, $tagId);
+                yield 'sRem' => $idList;
             }
         })->rewind();