[Cache] Handle APCu failures gracefully #23390
Conversation
| if (null !== $logger) { | ||
| if ('cli' === PHP_SAPI && !ini_get('apc.enable_cli')) { | ||
| $apcu->setLogger(new NullLogger()); | ||
| } elseif (null !== $logger) { |
There was a problem hiding this comment.
Why do we pass the NullLogger here? Why do we just not call setLogger() instead?
Like this:
if (null !== $logger && ('cli' !== PHP_SAPI || ini_get('apc.enable_cli'))) {
// ...
}There was a problem hiding this comment.
because when no logger is set, failures are still "logged" here:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/CacheItem.php#L182
but we really want to silence.
|
The changes look good at a first glance, but the tests are failing. |
|
👍 confirmed this solves the issue. |
nicolas-grekas
force-pushed
the
cache-apcu-fix
branch
from
89ba875
to
47020c4
Compare
|
Tests back to green. |
|
Will test this patch tomorrow to see if it works for my case |
|
Thank you @nicolas-grekas. |
This PR was merged into the 3.2 branch. Discussion ---------- [Cache] Handle APCu failures gracefully | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23344 | License | MIT | Doc PR | - When APCu memory is full, or when APCu is used in CLI but `apc.enable_cli` is off, it behaves in a special way that this PR now handles. When `apc.enable_cli` is off, we also completely silence failures with a `NullLogger` - that's just noise and that happens a lot during warmups, when filling the seeding FilesystemAdapter cache in the chain. Commits ------- 47020c4 [Cache] Handle APCu failures gracefully
|
This issue is fixed for me, thanks! |
|
Congratulations @nicolas-grekas ! 3.3.3 was absolutely not usable for me because of this issue, particularly on CircleCI (phpunit). So thank you very much. Fixed for me now, all is green. |
| @@ -92,7 +92,11 @@ protected function doDelete(array $ids) | |||
| protected function doSave(array $values, $lifetime) | |||
| { | |||
| try { | |||
| return array_keys(apcu_store($values, null, $lifetime)); | |||
| if (false === $failures = apcu_store($values, null, $lifetime)) { | |||
There was a problem hiding this comment.
Just curious, do you guys treat is a good-practice coding style? Most of linting tools warn you about assignments being done inside the conditional statements.
There was a problem hiding this comment.
Yes ... we decided a long ago (in 2011 if I remember correctly) to use Yoda-style conditions. I don't think we can change this now, because it will be a nightmare for our mergers.
…rixx) This PR was merged into the 3.2 branch. Discussion ---------- [Cache] Added test for ApcuAdapter when using in CLI | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23344 | License | MIT | Doc PR | - --- I also hit #23344 and I did not notice it was already fixed (#23390) and released (2 days ago, I updated the vendors 4 days ago). But as I have written test for it ... Let's contribute anyway Commits ------- 64d196a [Cache] Added test for ApcuAdapter when using in CLI
When APCu memory is full, or when APCu is used in CLI but
apc.enable_cliis off, it behaves in a special way that this PR now handles.When
apc.enable_cliis off, we also completely silence failures with aNullLogger- that's just noise and that happens a lot during warmups, when filling the seeding FilesystemAdapter cache in the chain.