[Cache] Unconditionally use PhpFilesAdapter for system pools by nicolas-grekas · Pull Request #27549 · symfony/symfony

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Cache] Unconditionally use PhpFilesAdapter for system pools#27549

[Cache] Unconditionally use PhpFilesAdapter for system pools#27549
fabpot merged 1 commit intosymfony:masterfrom
nicolas-grekas:cache-deprec-system-factory

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants