Skip to content
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.

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

[Cache] Handle APCu failures gracefully #23390

Merged
merged 1 commit into from Jul 5, 2017

Conversation

Projects
None yet
9 participants
@nicolas-grekas
Copy link
Member

commented Jul 4, 2017

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.

if (null !== $logger) {
if ('cli' === PHP_SAPI && !ini_get('apc.enable_cli')) {
$apcu->setLogger(new NullLogger());
} elseif (null !== $logger) {

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jul 4, 2017

Member

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'))) {
    // ...
}

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 4, 2017

Author Member

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.

@xabbuh xabbuh dismissed their stale review Jul 4, 2017

tests are failing

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

The changes look good at a first glance, but the tests are failing.

@yceruto

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2017

👍 confirmed this solves the issue.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-apcu-fix branch from 89ba875 to 47020c4 Jul 4, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2017

Tests back to green.

@xabbuh

xabbuh approved these changes Jul 4, 2017

@linaori

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2017

Will test this patch tomorrow to see if it works for my case

@fabpot

fabpot approved these changes Jul 5, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Jul 5, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 47020c4 into symfony:3.2 Jul 5, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jul 5, 2017

bug #23390 [Cache] Handle APCu failures gracefully (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
@linaori

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2017

This issue is fixed for me, thanks!

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:cache-apcu-fix branch Jul 5, 2017

This was referenced Jul 5, 2017

@ghost

This comment has been minimized.

Copy link

commented Jul 5, 2017

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)) {

This comment has been minimized.

Copy link
@rodush

rodush Jul 10, 2017

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.

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Jul 10, 2017

Member

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.

nicolas-grekas added a commit that referenced this pull request Jul 11, 2017

minor #23446 [Cache] Added test for ApcuAdapter when using in CLI (ly…
…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 :trollface: ). But as I have written test for it ... Let's contribute anyway

Commits
-------

64d196a [Cache] Added test for ApcuAdapter when using in CLI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.