Skip to content

[Cache] Always require symfony/polyfill-apcu to provide APCuIterator everywhere #24011

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

Merged
merged 1 commit into from
Aug 29, 2017
Merged

[Cache] Always require symfony/polyfill-apcu to provide APCuIterator everywhere #24011

merged 1 commit into from
Aug 29, 2017

Conversation

guillaumelecerf
Copy link

@guillaumelecerf guillaumelecerf commented Aug 28, 2017

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

TL;DR: when APCuIterator is not available (i.e. PHP 5.6 + APCu 4.0.7), the APC cache handling is broken.

When the app initializes one of the APC caches, it tries to retrieve its namespace flag key. If it can't, it clears its namespaced cache and adds its flag key back.
But when APCuIterator is not usable, the app flushes the whole cache each time, preventing all the APC caches to retrieve their own flag key, resulting in a perpetual full APC cache flush.

TODO:

See #24008

@chalasr
Copy link
Member

chalasr commented Aug 28, 2017

This updates the symfony/symfony deps, isn't the root issue part of Cache or FrameworkBundle?

@guillaumelecerf
Copy link
Author

@chalasr : fixed

@chalasr
Copy link
Member

chalasr commented Aug 28, 2017

If we are OK to make Cache requires polyfill-apcu (a bit weird to me since it can be used without apcu), a test case would be nice to prevent future regressions.

@guillaumelecerf
Copy link
Author

guillaumelecerf commented Aug 28, 2017

@chalasr : symfony/polyfill-apcu is used to provide APCuIterator for both PHP 5.5 + apcu 4.x and PHP 7.0.
See https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Traits/ApcuTrait.php#L74-L76

Will add a test ;)

@guillaumelecerf guillaumelecerf changed the title Always require symfony/polyfill-apcu to provide APCuIterator everywhere WIP: Always require symfony/polyfill-apcu to provide APCuIterator everywhere Aug 28, 2017
@guillaumelecerf guillaumelecerf changed the title WIP: Always require symfony/polyfill-apcu to provide APCuIterator everywhere Always require symfony/polyfill-apcu to provide APCuIterator everywhere Aug 28, 2017
@guillaumelecerf
Copy link
Author

guillaumelecerf commented Aug 28, 2017

Test added, but Travis does not report it as the tests are run with the require-dev dependencies, e.g. with symfony/polyfill-apcu.
I was able to make it fail by removing symfony/polyfill-apcu:
https://travis-ci.org/symfony/symfony/jobs/269383629#L3334

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Aug 29, 2017
@guillaumelecerf guillaumelecerf changed the title Always require symfony/polyfill-apcu to provide APCuIterator everywhere [Cache] Always require symfony/polyfill-apcu to provide APCuIterator everywhere Aug 29, 2017
@nicolas-grekas
Copy link
Member

the root composer file also needs to be patched as you did before

@guillaumelecerf
Copy link
Author

@nicolas-grekas : you mean both composer.json files ?

@nicolas-grekas
Copy link
Member

yes, both need the patch

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(note to mergers: polyfill-apcu should be removed when merging into master)

@fabpot
Copy link
Member

fabpot commented Aug 29, 2017

Thank you @guillaumelecerf.

@fabpot fabpot merged commit 9d44442 into symfony:3.3 Aug 29, 2017
fabpot added a commit that referenced this pull request Aug 29, 2017
…CuIterator everywhere (guillaumelecerf)

This PR was merged into the 3.3 branch.

Discussion
----------

[Cache] Always require symfony/polyfill-apcu to provide APCuIterator everywhere

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

**TL;DR: when APCuIterator is not available (i.e. PHP 5.6 + APCu 4.0.7), the APC cache handling is broken.**

When [the app initializes](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/cache.xml#L31) one of [the APC caches](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/cache.xml#L14-L28), it tries to [retrieve its namespace flag key](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Traits/ApcuTrait.php#L42). If it can't, [it clears its namespaced cache](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Traits/ApcuTrait.php#L43) and [adds its flag key back](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Traits/ApcuTrait.php#L44).
But [when APCuIterator is not usable](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Traits/ApcuTrait.php#L74), **the app [flushes the whole cache each time](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Traits/ApcuTrait.php#L76)**, preventing all the APC caches to retrieve their own flag key, resulting in a **perpetual full APC cache flush**.

TODO:
- [x] add test => https://travis-ci.org/symfony/symfony/jobs/269383629#L3334

See #24008

Commits
-------

9d44442 Always require symfony/polyfill-apcu to provide APCuIterator everywhere
@guillaumelecerf guillaumelecerf deleted the require_polyfill-apcu branch August 30, 2017 14:54
@fabpot fabpot mentioned this pull request Sep 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants