bug #17661 [Cache] Fix expiries handling (nicolas-grekas)

This PR was merged into the 3.1-dev branch. Discussion ---------- [Cache] Fix expiries handling | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 064ec0b [Cache] Fix expiries handling
symfony · Feb 8, 2016 · 00f5e7b · 00f5e7b
2 parents df99788 + 064ec0b
commit 00f5e7b
Show file tree

Hide file tree

Showing 9 changed files with 50 additions and 61 deletions.
diff --git a/src/Symfony/Component/Cache/Adapter/AbstractAdapter.php b/src/Symfony/Component/Cache/Adapter/AbstractAdapter.php
@@ -49,10 +49,13 @@ function ($key, $value, $isHit) use ($defaultLifetime) {
         $this->mergeByLifetime = \Closure::bind(
             function ($deferred, $namespace) {
                 $byLifetime = array();
+                $now = time();
 
                 foreach ($deferred as $key => $item) {
-                    if (0 <= $item->lifetime) {
-                        $byLifetime[(int) $item->lifetime][$namespace.$key] = $item->value;
+                    if (null === $item->expiry) {
+                        $byLifetime[0][$namespace.$key] = $item->value;
+                    } elseif ($item->expiry > $now) {
+                        $byLifetime[$item->expiry - $now][$namespace.$key] = $item->value;
                     }
                 }
 
@@ -166,20 +169,7 @@ public function hasItem($key)
         $id = $this->getId($key);
 
         if (isset($this->deferred[$key])) {
-            $item = (array) $this->deferred[$key];
-            try {
-                $e = null;
-                $value = $item[CacheItem::CAST_PREFIX.'value'];
-                $ok = $this->doSave(array($key => $value), $item[CacheItem::CAST_PREFIX.'lifetime']);
-                unset($this->deferred[$key]);
-
-                if (true === $ok || array() === $ok) {
-                    return true;
-                }
-            } catch (\Exception $e) {
-            }
-            $type = is_object($value) ? get_class($value) : gettype($value);
-            CacheItem::log($this->logger, 'Failed to save key "{key}" ({type})', array('key' => $key, 'type' => $type, 'exception' => $e));
+            $this->commit();
         }
 
         try {
@@ -286,44 +276,50 @@ public function saveDeferred(CacheItemInterface $item)
      */
     public function commit()
     {
-        $f = $this->mergeByLifetime;
-        $ko = array();
+        $ok = true;
+        $byLifetime = $this->mergeByLifetime;
+        $byLifetime = $byLifetime($this->deferred, $this->namespace);
+        $retry = $this->deferred = array();
 
-        foreach ($f($this->deferred, $this->namespace) as $lifetime => $values) {
+        foreach ($byLifetime as $lifetime => $values) {
             try {
-                if (true === $ok = $this->doSave($values, $lifetime)) {
-                    continue;
-                }
+                $e = $this->doSave($values, $lifetime);
             } catch (\Exception $e) {
-                $ok = false;
             }
-            if (false === $ok) {
-                $ok = array_keys($values);
+            if (true === $e || array() === $e) {
+                continue;
             }
-            foreach ($ok as $id) {
-                $ko[$lifetime][] = array($id => $values[$id]);
+            if (is_array($e) || 1 === count($values)) {
+                foreach (is_array($e) ? $e : array_keys($values) as $id) {
+                    $ok = false;
+                    $v = $values[$id];
+                    $type = is_object($v) ? get_class($v) : gettype($v);
+                    CacheItem::log($this->logger, 'Failed to save key "{key}" ({type})', array('key' => substr($id, strlen($this->namespace)), 'type' => $type, 'exception' => $e instanceof \Exception ? $e : null));
+                }
+            } else {
+                foreach ($values as $id => $v) {
+                    $retry[$lifetime][] = $id;
+                }
             }
         }
 
-        $this->deferred = array();
-        $ok = true;
-
         // When bulk-save failed, retry each item individually
-        foreach ($ko as $lifetime => $values) {
-            foreach ($values as $v) {
+        foreach ($retry as $lifetime => $ids) {
+            foreach ($ids as $id) {
                 try {
-                    $e = $this->doSave($v, $lifetime);
+                    $v = $byLifetime[$lifetime][$id];
+                    $e = $this->doSave(array($id => $v), $lifetime);
                 } catch (\Exception $e) {
                 }
-                if (true !== $e && array() !== $e) {
-                    $ok = false;
-                    foreach ($v as $key => $value) {
-                        $type = is_object($value) ? get_class($value) : gettype($value);
-                        CacheItem::log($this->logger, 'Failed to save key "{key}" ({type})', array('key' => $key, 'type' => $type, 'exception' => $e instanceof \Exception ? $e : null));
-                    }
+                if (true === $e || array() === $e) {
+                    continue;
                 }
+                $ok = false;
+                $type = is_object($v) ? get_class($v) : gettype($v);
+                CacheItem::log($this->logger, 'Failed to save key "{key}" ({type})', array('key' => substr($id, strlen($this->namespace)), 'type' => $type, 'exception' => $e instanceof \Exception ? $e : null));
             }
         }
+        $this->deferred = array();
 
         return $ok;
     }

diff --git a/src/Symfony/Component/Cache/Adapter/ApcuAdapter.php b/src/Symfony/Component/Cache/Adapter/ApcuAdapter.php
@@ -70,6 +70,6 @@ protected function doDelete(array $ids)
      */
     protected function doSave(array $values, $lifetime)
     {
-        return apcu_store($values, null, $lifetime);
+        return array_keys(apcu_store($values, null, $lifetime));
     }
 }
diff --git a/src/Symfony/Component/Cache/Adapter/ArrayAdapter.php b/src/Symfony/Component/Cache/Adapter/ArrayAdapter.php
@@ -78,7 +78,7 @@ public function getItems(array $keys = array())
             $this->validateKey($key);
         }
 
-        return $this->generateItems($keys);
+        return $this->generateItems($keys, time());
     }
 
     /**
@@ -132,9 +132,9 @@ public function save(CacheItemInterface $item)
         $item = (array) $item;
         $key = $item[CacheItem::CAST_PREFIX.'key'];
         $value = $item[CacheItem::CAST_PREFIX.'value'];
-        $lifetime = $item[CacheItem::CAST_PREFIX.'lifetime'];
+        $expiry = $item[CacheItem::CAST_PREFIX.'expiry'];
 
-        if (0 > $lifetime) {
+        if (null !== $expiry && $expiry <= time()) {
             return true;
         }
         if ($this->storeSerialized) {
@@ -149,7 +149,7 @@ public function save(CacheItemInterface $item)
         }
 
         $this->values[$key] = $value;
-        $this->expiries[$key] = $lifetime ? $lifetime + time() : PHP_INT_MAX;
+        $this->expiries[$key] = null !== $expiry ? $expiry : PHP_INT_MAX;
 
         return true;
     }
@@ -185,12 +185,12 @@ private function validateKey($key)
         return $key;
     }
 
-    private function generateItems(array $keys)
+    private function generateItems(array $keys, $now)
     {
         $f = $this->createCacheItem;
 
         foreach ($keys as $key) {
-            if (!$isHit = isset($this->expiries[$key]) && ($this->expiries[$key] >= time() || !$this->deleteItem($key))) {
+            if (!$isHit = isset($this->expiries[$key]) && ($this->expiries[$key] >= $now || !$this->deleteItem($key))) {
                 $value = null;
             } elseif ($this->storeSerialized) {
                 $value = unserialize($this->values[$key]);

diff --git a/src/Symfony/Component/Cache/Adapter/ProxyAdapter.php b/src/Symfony/Component/Cache/Adapter/ProxyAdapter.php
@@ -121,9 +121,10 @@ private function doSave(CacheItemInterface $item, $method)
             return false;
         }
         $item = (array) $item;
+        $expiry = $item[CacheItem::CAST_PREFIX.'expiry'];
         $poolItem = $this->pool->getItem($item[CacheItem::CAST_PREFIX.'key']);
         $poolItem->set($item[CacheItem::CAST_PREFIX.'value']);
-        $poolItem->expiresAfter($item[CacheItem::CAST_PREFIX.'lifetime']);
+        $poolItem->expiresAt(null !== $expiry ? \DateTime::createFromFormat('U', $expiry) : null);
 
         return $this->pool->$method($poolItem);
     }

diff --git a/src/Symfony/Component/Cache/CacheItem.php b/src/Symfony/Component/Cache/CacheItem.php
@@ -28,7 +28,7 @@ final class CacheItem implements CacheItemInterface
     private $key;
     private $value;
     private $isHit;
-    private $lifetime;
+    private $expiry;
     private $defaultLifetime;
 
     /**
@@ -71,9 +71,9 @@ public function set($value)
     public function expiresAt($expiration)
     {
         if (null === $expiration) {
-            $this->lifetime = $this->defaultLifetime;
+            $this->expiry = $this->defaultLifetime > 0 ? time() + $this->defaultLifetime : null;
         } elseif ($expiration instanceof \DateTimeInterface) {
-            $this->lifetime = $expiration->format('U') - time() ?: -1;
+            $this->expiry = $expiration->format('U');
         } else {
             throw new InvalidArgumentException(sprintf('Expiration date must implement DateTimeInterface or be null, "%s" given', is_object($expiration) ? get_class($expiration) : gettype($expiration)));
         }
@@ -87,12 +87,11 @@ public function expiresAt($expiration)
     public function expiresAfter($time)
     {
         if (null === $time) {
-            $this->lifetime = $this->defaultLifetime;
+            $this->expiry = $this->defaultLifetime > 0 ? time() + $this->defaultLifetime : null;
         } elseif ($time instanceof \DateInterval) {
-            $now = time();
-            $this->lifetime = \DateTime::createFromFormat('U', $now)->add($time)->format('U') - $now ?: -1;
+            $this->expiry = \DateTime::createFromFormat('U', time())->add($time)->format('U');
         } elseif (is_int($time)) {
-            $this->lifetime = $time ?: -1;
+            $this->expiry = $time + time();
         } else {
             throw new InvalidArgumentException(sprintf('Expiration date must be an integer, a DateInterval or null, "%s" given', is_object($time) ? get_class($time) : gettype($time)));
         }

diff --git a/src/Symfony/Component/Cache/Tests/Adapter/ApcuAdapterTest.php b/src/Symfony/Component/Cache/Tests/Adapter/ApcuAdapterTest.php
@@ -16,10 +16,6 @@
 
 class ApcuAdapterTest extends CachePoolTest
 {
-    protected $skippedTests = array(
-        'testDeferredExpired' => 'Failing for now, needs to be fixed.',
-    );
-
     public function createCachePool()
     {
         if (defined('HHVM_VERSION')) {

diff --git a/src/Symfony/Component/Cache/Tests/Adapter/ArrayAdapterTest.php b/src/Symfony/Component/Cache/Tests/Adapter/ArrayAdapterTest.php
@@ -22,7 +22,6 @@ class ArrayAdapterTest extends CachePoolTest
     protected $skippedTests = array(
         'testDeferredSaveWithoutCommit' => 'Assumes a shared cache which ArrayAdapter is not.',
         'testSaveWithoutExpire' => 'Assumes a shared cache which ArrayAdapter is not.',
-        'testDeferredExpired' => 'Failing for now, needs to be fixed.',
     );
 
     public function createCachePool()

diff --git a/src/Symfony/Component/Cache/Tests/Adapter/DoctrineAdapterTest.php b/src/Symfony/Component/Cache/Tests/Adapter/DoctrineAdapterTest.php
@@ -23,7 +23,6 @@ class DoctrineAdapterTest extends CachePoolTest
     protected $skippedTests = array(
         'testDeferredSaveWithoutCommit' => 'Assumes a shared cache which ArrayCache is not.',
         'testSaveWithoutExpire' => 'Assumes a shared cache which ArrayCache is not.',
-        'testDeferredExpired' => 'Failing for now, needs to be fixed.',
     );
 
     public function createCachePool()

diff --git a/src/Symfony/Component/Cache/Tests/Adapter/ProxyAdapterTest.php b/src/Symfony/Component/Cache/Tests/Adapter/ProxyAdapterTest.php
@@ -23,7 +23,6 @@ class ProxyAdapterTest extends CachePoolTest
     protected $skippedTests = array(
         'testDeferredSaveWithoutCommit' => 'Assumes a shared cache which ArrayAdapter is not.',
         'testSaveWithoutExpire' => 'Assumes a shared cache which ArrayAdapter is not.',
-        'testDeferredExpired' => 'Failing for now, needs to be fixed.',
     );
 
     public function createCachePool()