Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Cache] Use correct expiry in ChainAdapter#38635
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, this looks good to me. Now waiting for a test case :)
Uh oh!
There was an error while loading.Please reload this page.
Nyholm commentedOct 20, 2020
I added a test. I needed to use an adapter that extends AbstractAdapter. |
nicolas-grekas commentedOct 21, 2020
Thank you@Nyholm. |
72652c3 to17e0167CompareNyholm commentedOct 21, 2020
Thank you for merging. |
This PR was merged into the 4.4 branch.Discussion----------[Cache] Fixed broken test| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets || License | MIT| Doc PR |I added this line of code in#38635However, some time between 4.4 and 5.x the `FilesystemAdapterTest::rmdir()` was removed. This PR make sure tests does not fail on 5.x.I target 4.4, I believe it will be simple to merge up to 5.1 and 5.x. Let me know if I should target 5.x instead.Commits-------e17797c [Cache] Fixed broken test
| if (0 <$defaultLifetime) { | ||
| if (isset($item->metadata[CacheItem::METADATA_EXPIRY])) { | ||
| $item->expiresAt(\DateTime::createFromFormat('U.u',$item->metadata[CacheItem::METADATA_EXPIRY])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just update to latest release with this change and it broke my site. As i know the expiry value is always a float number without precision (microsecond part). UsesU.u causing problem here. The format should beU. Can you advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you elaborate whyU.u casing a problem?
It seams to work:https://3v4l.org/pdadj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please open an issue with the error message you see and a reproducer or a failing test case ideally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suppose we should usesprintf('%.3F', ...)
the issue happens when the expiry is an int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, in my case expiry was int, not float.
simple.cache: default_lifetime: 86400 adapters: - cache.adapter.apcu - cache.adapter.redispublic function isPartner(User $user): bool { return $this->simpleCache->get(md5((string)$user->getId() . __METHOD__), function (ItemInterface $item) use ($user) { $item->expiresAfter(86400 * 7); // 1 week $item->tag(md5((string)$user->getId())); // compute return false; }); }nicolas-grekasOct 28, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
thanks,@phamviet would you mind sending a PR with the fix and a test case, please?
for branch 4.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is middle-night my time and I would do it tomorrow if it is convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is hard to me to reproduce myself today and finally i found down the problem could happen with ArrayAdapter in a race condition making expiry to be int. I'm not sure how could it happen like that. BTW, the PR is at#38879.
…ace conditions (phamviet)This PR was merged into the 4.4 branch.Discussion----------[Cache] Fixed expiry could be int in ChainAdapter due to race conditions| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fix#38635| License | MIT| Doc PR | noThis bug is hard to re-produce and seems only happen with ArrayAdapter only.Steps to reproduce:cache.yaml```simple.cache: adapters: - cache.adapter.array - cache.adapter.redis``````phpif (isset($item->metadata[CacheItem::METADATA_EXPIRY])) { $logger->debug($item->key, $item->metadata); $format = is_int($item->metadata[CacheItem::METADATA_EXPIRY]) ? 'U' : 'U.u'; $item->expiresAt(\DateTime::createFromFormat($format, $item->metadata[CacheItem::METADATA_EXPIRY])); }```Refresh webpage multiple time to make web server busy and logs:```[2020-10-29T17:04:51.119653+07:00] application.DEBUG: item-key {"expiry":1603965892.118222,"ctime":4} [][2020-10-29T17:04:54.322937+07:00] application.DEBUG: item-key {"expiry":1603965895.308393,"ctime":17} [][2020-10-29T17:04:54.745923+07:00] application.DEBUG: item-key {"expiry":1603965895,"ctime":16} []```Commits-------268816f [Cache] Fixed expiry maybe int due too race conditions
Uh oh!
There was an error while loading.Please reload this page.
When we are syncing the chain, Let's use expiry if we have it. If not, fallback to defaultLifetime.
TODO: