[Cache] Use correct expiry in ChainAdapter #38635
Conversation
|
I added a test. I needed to use an adapter that extends AbstractAdapter. |
|
Thank you @Nyholm. |
72652c3 to
17e0167
Compare
|
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 #38635 However, 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
| @@ -73,7 +73,9 @@ static function ($sourceItem, $item, $sourceMetadata = null) use ($defaultLifeti | ||
| $item->isHit = $sourceItem->isHit; | ||
| $item->metadata = $item->newMetadata = $sourceItem->metadata = $sourceMetadata; | ||
|
|
||
| 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.
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). Uses U.u causing problem here. The format should be U. Can you advice?
There was a problem hiding this comment.
Can you elaborate why U.u casing a problem?
It seams to work: https://3v4l.org/pdadj
There was a problem hiding this comment.
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.
I suppose we should use sprintf('%.3F', ...)
the issue happens when the expiry is an int
There was a problem hiding this comment.
Yes, in my case expiry was int, not float.
simple.cache:
default_lifetime: 86400
adapters:
- cache.adapter.apcu
- cache.adapter.redis
public 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;
});
}
There was a problem hiding this comment.
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.
It is middle-night my time and I would do it tomorrow if it is convenient.
There was a problem hiding this comment.
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 | no This 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 ``` ```php if (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
When we are syncing the chain, Let's use expiry if we have it. If not, fallback to defaultLifetime.
TODO: