Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[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.

Sign up for GitHub

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
Oct 21, 2020
Merged

[Cache] Use correct expiry in ChainAdapter #38635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion 4 src/Symfony/Component/Cache/Adapter/ChainAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]));
Copy link
Contributor

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). Uses U.u causing problem here. The format should be U. Can you advice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate why U.u casing a problem?

It seams to work: https://3v4l.org/pdadj

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we should use sprintf('%.3F', ...)
the issue happens when the expiry is an int

Copy link
Contributor

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.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;
        });
    }

Copy link
Member

@nicolas-grekas nicolas-grekas Oct 28, 2020 •
edited
Loading

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

Copy link
Contributor

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.

Copy link
Contributor

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.

} elseif (0 < $defaultLifetime) {
$item->expiresAfter($defaultLifetime);
}

Expand Down
49 changes: 49 additions & 0 deletions 49 src/Symfony/Component/Cache/Tests/Adapter/ChainAdapterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
use Symfony\Component\Cache\Adapter\ArrayAdapter;
use Symfony\Component\Cache\Adapter\ChainAdapter;
use Symfony\Component\Cache\Adapter\FilesystemAdapter;
use Symfony\Component\Cache\CacheItem;
use Symfony\Component\Cache\Tests\Fixtures\ExternalAdapter;
use Symfony\Component\Cache\Tests\Fixtures\PrunableAdapter;
use Symfony\Contracts\Cache\ItemInterface;

/**
* @author Kévin Dunglas <dunglas@gmail.com>
Expand All @@ -34,6 +36,11 @@ public function createCachePool(int $defaultLifetime = 0, string $testMethod = n
return new ChainAdapter([new ArrayAdapter($defaultLifetime), new ExternalAdapter($defaultLifetime), new FilesystemAdapter('', $defaultLifetime)], $defaultLifetime);
}

public static function tearDownAfterClass(): void
{
FilesystemAdapterTest::rmdir(sys_get_temp_dir().'/symfony-cache');
}

public function testEmptyAdaptersException()
{
$this->expectException('Symfony\Component\Cache\Exception\InvalidArgumentException');
Expand Down Expand Up @@ -187,6 +194,48 @@ public function testMultipleCachesExpirationWhenCommonTtlIsSet()
$this->assertFalse($item->isHit());
}

public function testExpirationOnAllAdapters()
{
if (isset($this->skippedTests[__FUNCTION__])) {
$this->markTestSkipped($this->skippedTests[__FUNCTION__]);
}

$itemValidator = function (CacheItem $item) {
$refl = new \ReflectionObject($item);
$propExpiry = $refl->getProperty('expiry');
$propExpiry->setAccessible(true);
$expiry = $propExpiry->getValue($item);
$this->assertGreaterThan(10, $expiry - time(), 'Item should be saved with the given ttl, not the default for the adapter.');

return true;
};

$adapter1 = $this->getMockBuilder(FilesystemAdapter::class)
->setConstructorArgs(['', 2])
->setMethods(['save'])
->getMock();
$adapter1->expects($this->once())
->method('save')
->with($this->callback($itemValidator))
->willReturn(true);

$adapter2 = $this->getMockBuilder(FilesystemAdapter::class)
->setConstructorArgs(['', 4])
->setMethods(['save'])
->getMock();
$adapter2->expects($this->once())
->method('save')
->with($this->callback($itemValidator))
->willReturn(true);

$cache = new ChainAdapter([$adapter1, $adapter2], 6);
$cache->get('test_key', function (ItemInterface $item) {
$item->expiresAfter(15);

return 'chain';
});
}

private function getPruneableMock(): AdapterInterface
{
$pruneable = $this->createMock(PrunableAdapter::class);
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.