bug #30576 [Cache] fix LockRegistry (nicolas-grekas)

This PR was merged into the 4.2 branch.

Discussion
----------

[Cache] fix LockRegistry

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

- Locking doesn't work right now, because of this missing return statement.
I can't provide a useful test case, because this is effective only when race conditions happen...
- Writing the value should happen within the boundaries of the lock.
- The `final` keyword is really missing, there are zero reasons to extend this class, adding it asap will make it clear and will unlock progress on this class.
- Lastly, a type-hint fix is shipped here also.

Commits
-------

f49df4a [Cache] fix LockRegistry

src/Symfony/Component/Cache/LockRegistry.php

@@ -23,7 +23,7 @@
  *
  * @author Nicolas Grekas <p@tchwork.com>
  */
-class LockRegistry
+final class LockRegistry
 {
     private static $openedFiles = [];
     private static $lockedFiles = [];
@@ -74,7 +74,7 @@ public static function setFiles(array $files): array
         return $previousFiles;
     }
 
-    public static function compute(callable $callback, ItemInterface $item, bool &$save, CacheInterface $pool)
+    public static function compute(callable $callback, ItemInterface $item, bool &$save, CacheInterface $pool, \Closure $setMetadata = null)
     {
         $key = self::$files ? crc32($item->getKey()) % \count(self::$files) : -1;
 
@@ -88,7 +88,18 @@ public static function compute(callable $callback, ItemInterface $item, bool &$s
                 if (flock($lock, LOCK_EX | LOCK_NB)) {
                     self::$lockedFiles[$key] = true;
 
-                    return $callback($item, $save);
+                    $value = $callback($item, $save);
+
+                    if ($save) {
+                        if ($setMetadata) {
+                            $setMetadata($item);
+                        }
+
+                        $pool->save($item->set($value));
+                        $save = false;
+                    }
+
+                    return $value;
                 }
                 // if we failed the race, retry locking in blocking mode to wait for the winner
                 flock($lock, LOCK_SH);
@@ -125,6 +136,6 @@ private static function open(int $key)
             restore_error_handler();
         }
 
-        self::$openedFiles[$key] = $h ?: @fopen(self::$files[$key], 'r');
+        return self::$openedFiles[$key] = $h ?: @fopen(self::$files[$key], 'r');
     }
 }

src/Symfony/Component/Cache/Traits/ContractsTrait.php

@@ -40,7 +40,7 @@ trait ContractsTrait
     public function setCallbackWrapper(?callable $callbackWrapper): callable
     {
         $previousWrapper = $this->callbackWrapper;
-        $this->callbackWrapper = $callbackWrapper ?? function (callable $callback, ItemInterface $item, bool &$save, CacheInterface $pool) {
+        $this->callbackWrapper = $callbackWrapper ?? function (callable $callback, ItemInterface $item, bool &$save, CacheInterface $pool, \Closure $setMetadata) {
             return $callback($item, $save);
         };
 
@@ -56,17 +56,19 @@ private function doGet(AdapterInterface $pool, string $key, callable $callback,
         static $setMetadata;
 
         $setMetadata = $setMetadata ?? \Closure::bind(
-            function (AdapterInterface $pool, ItemInterface $item, float $startTime) {
+            function (CacheItem $item, float $startTime, ?array &$metadata) {
                 if ($item->expiry > $endTime = microtime(true)) {
-                    $item->newMetadata[ItemInterface::METADATA_EXPIRY] = $item->expiry;
-                    $item->newMetadata[ItemInterface::METADATA_CTIME] = 1000 * (int) ($endTime - $startTime);
+                    $item->newMetadata[CacheItem::METADATA_EXPIRY] = $metadata[CacheItem::METADATA_EXPIRY] = $item->expiry;
+                    $item->newMetadata[CacheItem::METADATA_CTIME] = $metadata[CacheItem::METADATA_CTIME] = 1000 * (int) ($endTime - $startTime);
+                } else {
+                    unset($metadata[CacheItem::METADATA_EXPIRY], $metadata[CacheItem::METADATA_CTIME]);
                 }
             },
             null,
             CacheItem::class
         );
 
-        return $this->contractsGet($pool, $key, function (CacheItem $item, bool &$save) use ($pool, $callback, $setMetadata) {
+        return $this->contractsGet($pool, $key, function (CacheItem $item, bool &$save) use ($pool, $callback, $setMetadata, &$metadata) {
             // don't wrap nor save recursive calls
             if (null === $callbackWrapper = $this->callbackWrapper) {
                 $value = $callback($item, $save);
@@ -78,8 +80,10 @@ function (AdapterInterface $pool, ItemInterface $item, float $startTime) {
             $startTime = microtime(true);
 
             try {
-                $value = $callbackWrapper($callback, $item, $save, $pool);
-                $setMetadata($pool, $item, $startTime);
+                $value = $callbackWrapper($callback, $item, $save, $pool, function (CacheItem $item) use ($setMetadata, $startTime, &$metadata) {
+                    $setMetadata($item, $startTime, $metadata);
+                });
+                $setMetadata($item, $startTime, $metadata);
 
                 return $value;
             } finally {