10000 [Cache] Unconditionally use PhpFilesAdapter for system pools by nicolas-grekas · Pull Request #27549 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Cache] Unconditionally use PhpFilesAdapter for system pools #27549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 11, 2018
Merged

[Cache] Unconditionally use PhpFilesAdapter for system pools #27549

merged 1 commit into from
Jun 11, 2018

Conversation

nicolas-grekas
Copy link
Member
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.

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 7, 2018
@nicolas-grekas nicolas-grekas force-pushed the cache-deprec-system-factory branch from b2b40e4 to 8cf36c7 Compare June 7, 2018 21:33
@palex-fpt
Copy link

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 nicolas-grekas force-pushed the cache-deprec-system-factory branch from 8cf36c7 to 19c30c9 Compare June 8, 2018 05:46
@nicolas-grekas
Copy link
Member Author

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
Copy link
palex-fpt commented Jun 8, 2018 •
edited
Loading

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

@nicolas-grekas nicolas-grekas force-pushed the cache-deprec-system-factory branch from 19c30c9 to df6dddb Compare June 8, 2018 06:00
@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Jun 8, 2018 •
edited
Loading

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 nicolas-grekas force-pushed the cache-deprec-system-factory branch from df6dddb to 647cfcc Compare June 8, 2018 06:13
@nicolas-grekas
Copy link
Member Author

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
Copy link

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
Copy link
palex-fpt commented Jun 8, 2018 •
edited
Loading 
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 nicolas-grekas force-pushed the cache-deprec-system-factory branch from 647cfcc to 22bf5c3 Compare June 8, 2018 06:54
@nicolas-grekas
Copy link
Member Author

Thanks, patch applied.

@palex-fpt
Copy link

opcache_invalidate should be called before unlink

@palex-fpt
8000 Copy link

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

@nicolas-grekas 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
Copy link
Member Author

Thanks, now green!

@nicolas-grekas nicolas-grekas force-pushed the cache-deprec-system-factory branch from cc1edab to ce1a15d Compare June 8, 2018 20:30
@nicolas-grekas nicolas-grekas mentioned this pull request Jun 8, 2018
Merged
nicolas-grekas
nicolas-grekas commented Jun 8, 2018
View reviewed changes
src/Symfony/Component/Cache/Adapter/PhpArrayAdapter.php
@@ -43,7 +43,6 @@ public function __construct(string $file, AdapterInterface $fallbackPool)
{
$this->file = $file;
$this->pool = $fallbackPool;
$this->zendDetectUnicode = ini_get('zend.detect_unicode');
Copy link
Member Author
@nicolas-grekas nicolas-grekas Jun 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nicolas-grekas nicolas-grekas force-pushed the cache-deprec-system-factory branch from ce1a15d to c91b778 Compare June 8, 2018 22:53
fabpot
fabpot approved these changes Jun 11, 2018
View reviewed changes
src/Symfony/Component/Cache/Adapter/AbstractAdapter.php Outdated
@@ -87,9 +87,13 @@ function ($deferred, $namespace, &$expiredIds) use ($getId) {
* @param LoggerInterface|null $logger
*
* @return AdapterInterface
*
* @deprecated since Symfony 4.2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@nicolas-grekas nicolas-grekas force-pushed the cache-deprec-system-factory branch from c91b778 to 51381e5 Compare June 11, 2018 07:58
@fabpot
Copy link
Member
fabpot commented Jun 11, 2018

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 51381e5 into symfony:master Jun 11, 2018
fabpot added a commit that referenced this pull request Jun 11, 2018
@fabpot
feature #27549 [Cache] Unconditionally use PhpFilesAdapter for system…
d4f5d46 
… 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
@nicolas-grekas nicolas-grekas deleted the cache-deprec-system-factory branch June 11, 2018 09:02
stof
stof reviewed Jun 11, 2018
View reviewed changes
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be __METHOD__ ?

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Oct 10, 2018
@nicolas-grekas
Revert "feature symfony#27549 [Cache] Unconditionally use PhpFilesAda…
dbc1230 
…pter for system pools (nicolas-grekas)"

This reverts commit d4f5d46, reversing
changes made to 7e3b7b0.
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 10, 2018
Merged
fabpot added a commit that referenced this pull request Oct 10, 2018
67ED
@fabpot
bug #28805 Revert "feature #27549 [Cache] Unconditionally use PhpFile…
9a37ba4 
…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 nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
Closed
Merged
@PabloKowalczyk PabloKowalczyk mentioned this pull request Nov 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Cache Deprecation Status: Reviewed
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
5 participants
@nicolas-grekas @palex-fpt @fabpot @stof @carsonbot
0