[Cache] Unconditionally use PhpFilesAdapter for system pools #27549

nicolas-grekas · 2018-06-07T21:28:06Z

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

Now that we're about to leverage OPCache shared memory even for objects (see #27543), there's no reason anymore to use APCu for system caches. Let's remove some complexity here.

As a bonus, this makes system caches pruneable using the cache:pool:prune command.

palex-fpt · 2018-06-08T00:48:17Z

There is one problem with PhpFilesAdapter - it does not use opcache till next request.

 $cache->set('key', 'value');
 $cache->get('key'); // load directly from file, as file with timestamp >= request start time is not opcached.

nicolas-grekas · 2018-06-08T05:48:34Z

There is one problem with PhpFilesAdapter - it does not use opcache till next request.

After having a deeper look, it does, but only if the file's mtime is in the past. This is now fixed. Thanks for the hint.

palex-fpt · 2018-06-08T05:52:24Z

to pass tests it should invalidate cache on file deletion (opcache_invalidate should be called before file is unlinked)

nicolas-grekas · 2018-06-08T06:03:28Z

to pass tests it should invalidate cache on file deletion (opcache_invalidate should be called before file is unlinked)

That would open race conditions (invalidating, then a concurrent request reloads the legacy file, then we write but this is ignored). ~~Tests do pass so not sure what you meant~~ (hum, I missed the failures on FrameworkBundle, I'll have a look.)

nicolas-grekas · 2018-06-08T06:21:22Z

Hum, dunno why but tests pass locally and not the CI. I added the call you suggested but still red. Any idea why?

palex-fpt · 2018-06-08T06:27:11Z

calls to $cache->delete() or $cache->clear() unlinks files. but opcache is still has file cached. And list($expiresAt, $values[$id]) = include $file; returns data. Tests pass when opcache is not working (opcache_cli = false)

palex-fpt · 2018-06-08T06:38:08Z

Index: src/Symfony/Component/Cache/Traits/PhpFilesTrait.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/Symfony/Component/Cache/Traits/PhpFilesTrait.php	(revision 3a2eb0d9514f0b322ab31a128aa48b9f89e28d50)
+++ src/Symfony/Component/Cache/Traits/PhpFilesTrait.php	(date 1528439846061)
@@ -23,7 +23,9 @@
  */
 trait PhpFilesTrait
 {
-    use FilesystemCommonTrait;
+    use FilesystemCommonTrait {
+        doUnlink as doFsUnlink;
+    }
 
     private $includeHandler;
     private $zendDetectUnicode;
@@ -155,4 +157,13 @@
 
         return $ok;
     }
+
+    protected function doUnlink($file)
+    {
+        $allowCompile = 'cli' !== PHP_SAPI || ini_get('opcache.enable_cli');
+        if ($allowCompile) {
+            @opcache_invalidate($file, true);
+        }
+        return $this->doFsUnlink($file);
+    }
 }
Index: src/Symfony/Component/Cache/Traits/FilesystemCommonTrait.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/Symfony/Component/Cache/Traits/FilesystemCommonTrait.php	(revision 3a2eb0d9514f0b322ab31a128aa48b9f89e28d50)
+++ src/Symfony/Component/Cache/Traits/FilesystemCommonTrait.php	(date 1528439709379)
@@ -56,7 +56,7 @@
         $ok = true;
 
         foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($this->directory, \FilesystemIterator::SKIP_DOTS)) as $file) {
-            $ok = ($file->isDir() || @unlink($file) || !file_exists($file)) && $ok;
+            $ok = ($file->isDir() || @$this->doUnlink($file) || !file_exists($file)) && $ok;
         }
 
         return $ok;
@@ -71,7 +71,7 @@
 
         foreach ($ids as $id) {
             $file = $this->getFile($id);
-            $ok = (!file_exists($file) || @unlink($file) || !file_exists($file)) && $ok;
+            $ok = (!file_exists($file) || @$this->doUnlink($file) || !file_exists($file)) && $ok;
         }
 
         return $ok;
@@ -122,7 +122,12 @@
             parent::__destruct();
         }
         if (null !== $this->tmp && file_exists($this->tmp)) {
-            unlink($this->tmp);
+            $this->doUnlink($this->tmp);
         }
     }
+
+    protected function doUnlink($file)
+    {
+        return @unlink($file);
+    }
 }

nicolas-grekas · 2018-06-08T06:54:31Z

Thanks, patch applied.

palex-fpt · 2018-06-08T06:58:22Z

opcache_invalidate should be called before unlink

palex-fpt · 2018-06-08T07:00:14Z

invalidate fails when file does not exits and opcache still contains file entry.

nicolas-grekas · 2018-06-08T11:21:15Z

Thanks, now green!

nicolas-grekas · 2018-06-08T20:59:52Z

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

    {
        $this->file = $file;
        $this->pool = $fallbackPool;
-        $this->zendDetectUnicode = ini_get('zend.detect_unicode');


No need to deal with zend.detect_unicode anymore when this line is removed. Let's remove the related complexity and overhead.

fabpot · 2018-06-11T07:26:57Z

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

     *
     * @return AdapterInterface
+     *
+     * @deprecated since Symfony 4.2.


no dot at the end (I know, our conventions are hard to follow :)).

fabpot · 2018-06-11T08:38:37Z

Thank you @nicolas-grekas.

… pools (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Cache] Unconditionally use PhpFilesAdapter for system pools | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Now that we're about to leverage OPCache shared memory even for objects (see #27543), there's no reason anymore to use APCu for system caches. Let's remove some complexity here. As a bonus, this makes system caches pruneable using the `cache:pool:prune` command. Commits ------- 51381e5 [Cache] Unconditionally use PhpFilesAdapter for system pools

stof · 2018-06-11T09:51:27Z

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

     */
    public static function createSystemCache($namespace, $defaultLifetime, $version, $directory, LoggerInterface $logger = null)
    {
+        @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.2.', __CLASS__), E_USER_DEPRECATED);


shouldn't it be __METHOD__ ?

…pter for system pools (nicolas-grekas)" This reverts commit d4f5d46, reversing changes made to 7e3b7b0.

…sAdapter for system pools" (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- Revert "feature #27549 [Cache] Unconditionally use PhpFilesAdapter for system pools" This reverts commit d4f5d46, reversing changes made to 7e3b7b0. | 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 | - Reading #28800, I've just realized that #27549 breaks using system caches with read-only filesystem. Using ApcuAdapter makes system caches compatible with read-only filesystems. Note that this affects only non-warmed up pools, as the warmed-up ones use a faster `PhpArrayAdapter` in front. Commits ------- dbc1230 Revert "feature #27549 [Cache] Unconditionally use PhpFilesAdapter for system pools (nicolas-grekas)"

nicolas-grekas added this to the next milestone Jun 7, 2018

carsonbot added Status: Needs Review Cache Deprecation labels Jun 7, 2018

nicolas-grekas force-pushed the cache-deprec-system-factory branch from b2b40e4 to 8cf36c7 Compare June 7, 2018 21:33

nicolas-grekas force-pushed the cache-deprec-system-factory branch from 8cf36c7 to 19c30c9 Compare June 8, 2018 05:46

nicolas-grekas force-pushed the cache-deprec-system-factory branch from 19c30c9 to df6dddb Compare June 8, 2018 06:00

nicolas-grekas force-pushed the cache-deprec-system-factory branch from df6dddb to 647cfcc Compare June 8, 2018 06:13

nicolas-grekas force-pushed the cache-deprec-system-factory branch from 647cfcc to 22bf5c3 Compare June 8, 2018 06:54

nicolas-grekas force-pushed the cache-deprec-system-factory branch 6 times, most recently from d1f6da9 to cc1edab Compare June 8, 2018 11:09

nicolas-grekas force-pushed the cache-deprec-system-factory branch from cc1edab to ce1a15d Compare June 8, 2018 20:30

nicolas-grekas mentioned this pull request Jun 8, 2018

[Cache] serialize objects using native arrays when possible #27543

Merged

nicolas-grekas commented Jun 8, 2018

View reviewed changes

nicolas-grekas force-pushed the cache-deprec-system-factory branch from ce1a15d to c91b778 Compare June 8, 2018 22:53

fabpot approved these changes Jun 11, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 11, 2018

[Cache] Unconditionally use PhpFilesAdapter for system pools

51381e5

nicolas-grekas force-pushed the cache-deprec-system-factory branch from c91b778 to 51381e5 Compare June 11, 2018 07:58

fabpot merged commit 51381e5 into symfony:master Jun 11, 2018

nicolas-grekas deleted the cache-deprec-system-factory branch June 11, 2018 09:02

stof reviewed Jun 11, 2018

View reviewed changes

nicolas-grekas mentioned this pull request Oct 10, 2018

Revert "feature #27549 [Cache] Unconditionally use PhpFilesAdapter for system pools" #28805

Merged

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

PabloKowalczyk mentioned this pull request Nov 8, 2018

[FrameworkBundle][PropertyAccess][Cache] Use PhpFilesAdapter instead of ApcuAdapter #29143

Closed

Uh oh!

[Cache] Unconditionally use PhpFilesAdapter for system pools #27549

[Cache] Unconditionally use PhpFilesAdapter for system pools #27549

Uh oh!

Conversation

nicolas-grekas commented Jun 7, 2018

Uh oh!

palex-fpt commented Jun 8, 2018

Uh oh!

nicolas-grekas commented Jun 8, 2018

Uh oh!

palex-fpt commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jun 8, 2018

Uh oh!

palex-fpt commented Jun 8, 2018

Uh oh!

palex-fpt commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jun 8, 2018

Uh oh!

palex-fpt commented Jun 8, 2018

Uh oh!

palex-fpt commented Jun 8, 2018

Uh oh!

nicolas-grekas commented Jun 8, 2018

Uh oh!

nicolas-grekas Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jun 11, 2018

Uh oh!

stof Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

palex-fpt commented Jun 8, 2018 •

edited

Loading

nicolas-grekas commented Jun 8, 2018 •

edited

Loading

palex-fpt commented Jun 8, 2018 •

edited

Loading

nicolas-grekas Jun 8, 2018 •

edited

Loading