Skip to content

Commit

Permalink
bug #46699 [Cache] Respect $save option in all adapters (jrjohnson)
Browse files Browse the repository at this point in the history
This PR was merged into the 4.4 branch.

Discussion
----------

[Cache] Respect $save option in all adapters

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

I was working with a cache chain which included the array adapter. When we have a cache miss we need to consolidate all of the misses so we can do a single DB query for all values. So first we look in the cache, then and then we can pull the data as a big group, hydrate the results, and then cache them. Because we search for these values a bunch of times in the same request I chained the array and Redis adapter together.

Looks like:
```
$hitsAndMisses = array_map(
  fn(mixed $id) => $this->cache->get($id, function (ItemInterface $item, bool &$save) {
    $save = false;
    return false;
  }),
  $ids
);

 $hits = array_filter($hitsAndMisses);

//cut: compare arrays to get missed Ids

$hydrated = $this->hydrate($missedIds); //very expensive to do individually

$missedValues = array_map(fn($obj) => $this->cache->get(
  $obj->id,
  function (ItemInterface $item) use ($obj) {
    return $obj;
  }
), $hydrated);

return array_values([...$hits, ...$missedValues]);
```

Unfortunately neither the `ArrayAdapter` nor the `ChainAdapter` respect the $save reference. I was able to fix the `ArrayAdapter` in this PR, but I'm at a loss for how to do the same for the `ChainAdapter`, but I was able to create a generic failing test.

Commits
-------

b14aa77 [Cache] Respect $save option in ChainAdapter
5e8de1e [Cache] Respect $save option in ArrayAdapter
  • Loading branch information
nicolas-grekas committed Jun 19, 2022
2 parents 5990514 + b14aa77 commit fa4a7a4
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 2 deletions.
5 changes: 4 additions & 1 deletion src/Symfony/Component/Cache/Adapter/ArrayAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ public function get(string $key, callable $callback, float $beta = null, array &
// ArrayAdapter works in memory, we don't care about stampede protection
if (\INF === $beta || !$item->isHit()) {
$save = true;
$this->save($item->set($callback($item, $save)));
$item->set($callback($item, $save));
if ($save) {
$this->save($item);
}
}

return $item->get();
Expand Down
11 changes: 10 additions & 1 deletion src/Symfony/Component/Cache/Adapter/ChainAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,17 @@ static function ($sourceItem, $item, $sourceMetadata = null) use ($defaultLifeti
*/
public function get(string $key, callable $callback, float $beta = null, array &$metadata = null)
{
$doSave = true;
$callback = static function (CacheItem $item, bool &$save) use ($callback, &$doSave) {
$value = $callback($item, $save);
$doSave = $save;

return $value;
};

$lastItem = null;
$i = 0;
$wrap = function (CacheItem $item = null) use ($key, $callback, $beta, &$wrap, &$i, &$lastItem, &$metadata) {
$wrap = function (CacheItem $item = null, bool &$save = true) use ($key, $callback, $beta, &$wrap, &$i, &$doSave, &$lastItem, &$metadata) {
$adapter = $this->adapters[$i];
if (isset($this->adapters[++$i])) {
$callback = $wrap;
Expand All @@ -107,6 +115,7 @@ public function get(string $key, callable $callback, float $beta = null, array &
if (null !== $item) {
($this->syncItem)($lastItem = $lastItem ?? $item, $item, $metadata);
}
$save = $doSave;

return $value;
};
Expand Down
25 changes: 25 additions & 0 deletions src/Symfony/Component/Cache/Tests/Adapter/AdapterTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,31 @@ public function testRecursiveGet()
$this->assertSame(1, $cache->get('k2', function () { return 2; }));
}

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

$cache = $this->createCachePool(0, __FUNCTION__);

$v1 = $cache->get('some-key', function($item, &$save){
$save = false;
return 1;
});
$this->assertSame($v1, 1);

$v2 = $cache->get('some-key', function(){
return 2;
});
$this->assertSame($v2, 2, 'First value was cached and should not have been');

$v3 = $cache->get('some-key', function(){
$this->fail('Value should have come from cache');
});
$this->assertSame($v3, 2);
}

public function testGetMetadata()
{
if (isset($this->skippedTests[__FUNCTION__])) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class PhpArrayAdapterTest extends AdapterTestCase
{
protected $skippedTests = [
'testGet' => 'PhpArrayAdapter is read-only.',
'testDontSaveWhenAskedNotTo' => 'PhpArrayAdapter is read-only.',
'testRecursiveGet' => 'PhpArrayAdapter is read-only.',
'testBasicUsage' => 'PhpArrayAdapter is read-only.',
'testBasicUsageWithLongKey' => 'PhpArrayAdapter is read-only.',
Expand Down

0 comments on commit fa4a7a4

Please sign in to comment.