From f0950d910ce42d289363c08300488557fad4742c Mon Sep 17 00:00:00 2001 From: Matt Ford Date: Thu, 17 Aug 2023 16:26:40 +0100 Subject: [PATCH 1/8] Fix memory leak on flush method --- src/Illuminate/Cache/TaggedCache.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/Illuminate/Cache/TaggedCache.php b/src/Illuminate/Cache/TaggedCache.php index 7cd12303882c..d4bd9c603927 100644 --- a/src/Illuminate/Cache/TaggedCache.php +++ b/src/Illuminate/Cache/TaggedCache.php @@ -2,6 +2,8 @@ namespace Illuminate\Cache; +use Illuminate\Cache\Events\KeyForgotten; +use Illuminate\Cache\Events\KeyWritten; use Illuminate\Contracts\Cache\Store; class TaggedCache extends Repository @@ -17,6 +19,12 @@ class TaggedCache extends Repository */ protected $tags; + /** + * List of keys within this namespace + * @var array + */ + protected $itemKeys = []; + /** * Create a new tagged cache instance. * @@ -78,6 +86,8 @@ public function decrement($key, $value = 1) */ public function flush() { + array_walk($this->itemKeys, [$this->store, 'forget']); + $this->itemKeys = []; $this->tags->reset(); return true; @@ -110,6 +120,14 @@ public function taggedItemKey($key) */ protected function event($event) { + $itemKey = $this->itemKey($event->key); + if ($event instanceof KeyWritten && !in_array($itemKey, $this->itemKeys)) { + $this->itemKeys[] = $itemKey; + } elseif ($event instanceof KeyForgotten && in_array($itemKey, $this->itemKeys)) { + $this->itemKeys = array_values(array_filter($this->itemKeys, function ($k) use ($itemKey) { + return $k !== $itemKey; + })); + } parent::event($event->setTags($this->tags->getNames())); } From 63632e3c9a4e518a33841f4f6bb9f6775e978e41 Mon Sep 17 00:00:00 2001 From: Matt Ford Date: Thu, 17 Aug 2023 16:40:07 +0100 Subject: [PATCH 2/8] Style fixes --- src/Illuminate/Cache/TaggedCache.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Illuminate/Cache/TaggedCache.php b/src/Illuminate/Cache/TaggedCache.php index d4bd9c603927..971405a0d6e8 100644 --- a/src/Illuminate/Cache/TaggedCache.php +++ b/src/Illuminate/Cache/TaggedCache.php @@ -21,6 +21,7 @@ class TaggedCache extends Repository /** * List of keys within this namespace + * * @var array */ protected $itemKeys = []; @@ -121,7 +122,7 @@ public function taggedItemKey($key) protected function event($event) { $itemKey = $this->itemKey($event->key); - if ($event instanceof KeyWritten && !in_array($itemKey, $this->itemKeys)) { + if ($event instanceof KeyWritten && ! in_array($itemKey, $this->itemKeys)) { $this->itemKeys[] = $itemKey; } elseif ($event instanceof KeyForgotten && in_array($itemKey, $this->itemKeys)) { $this->itemKeys = array_values(array_filter($this->itemKeys, function ($k) use ($itemKey) { From a1dafdbe24c5b4839d1da03e51dc32e706611a15 Mon Sep 17 00:00:00 2001 From: Matt Ford Date: Thu, 17 Aug 2023 16:40:56 +0100 Subject: [PATCH 3/8] Add full stop. --- src/Illuminate/Cache/TaggedCache.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Illuminate/Cache/TaggedCache.php b/src/Illuminate/Cache/TaggedCache.php index 971405a0d6e8..d487000d6073 100644 --- a/src/Illuminate/Cache/TaggedCache.php +++ b/src/Illuminate/Cache/TaggedCache.php @@ -20,7 +20,7 @@ class TaggedCache extends Repository protected $tags; /** - * List of keys within this namespace + * List of keys within this namespace. * * @var array */ From 1d35633068035b055ccd011e7ce0845358aae0f3 Mon Sep 17 00:00:00 2001 From: Matt Ford Date: Fri, 18 Aug 2023 09:08:13 +0100 Subject: [PATCH 4/8] Change approach + add test --- src/Illuminate/Cache/TaggedCache.php | 45 +++++++++++++++++++++++----- tests/Cache/CacheTaggedCacheTest.php | 13 ++++++++ 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/Illuminate/Cache/TaggedCache.php b/src/Illuminate/Cache/TaggedCache.php index d487000d6073..4f4463276c69 100644 --- a/src/Illuminate/Cache/TaggedCache.php +++ b/src/Illuminate/Cache/TaggedCache.php @@ -87,8 +87,10 @@ public function decrement($key, $value = 1) */ public function flush() { - array_walk($this->itemKeys, [$this->store, 'forget']); - $this->itemKeys = []; + foreach ($this->getItemKeys() as $key) { + $this->store->forget($key); + } + $this->store->forget($this->getMetadataKey()); $this->tags->reset(); return true; @@ -122,16 +124,43 @@ public function taggedItemKey($key) protected function event($event) { $itemKey = $this->itemKey($event->key); - if ($event instanceof KeyWritten && ! in_array($itemKey, $this->itemKeys)) { - $this->itemKeys[] = $itemKey; - } elseif ($event instanceof KeyForgotten && in_array($itemKey, $this->itemKeys)) { - $this->itemKeys = array_values(array_filter($this->itemKeys, function ($k) use ($itemKey) { - return $k !== $itemKey; - })); + if ($itemKey !== $this->getMetadataKey() && ($event instanceof KeyWritten || $event instanceof KeyForgotten)) { + $itemKeys = $this->getItemKeys(); + if ($event instanceof KeyWritten && !in_array($itemKey, $itemKeys)) { + $itemKeys[] = $itemKey; + } elseif ($event instanceof KeyForgotten && in_array($itemKey, $this->itemKeys)) { + $itemKeys = array_values( + array_filter($itemKeys, function ($k) use ($itemKey) { + return $k !== $itemKey; + }) + ); + } + $this->putItemKeys($itemKeys); } parent::event($event->setTags($this->tags->getNames())); } + private function getMetadataKey(): string + { + return $this->taggedItemKey('meta:keys'); + } + + private function getItemKeys(): array + { + $metadataKey = $this->getMetadataKey(); + $keys = $this->store->get($metadataKey); + if (! is_array($keys)) { + $keys = []; + } + return $keys; + } + + private function putItemKeys(array $keys): void + { + $metadataKey = $this->getMetadataKey(); + $this->store->forever($metadataKey, $keys); + } + /** * Get the tag set instance. * diff --git a/tests/Cache/CacheTaggedCacheTest.php b/tests/Cache/CacheTaggedCacheTest.php index 1834bce56d4f..7ee140c718e3 100644 --- a/tests/Cache/CacheTaggedCacheTest.php +++ b/tests/Cache/CacheTaggedCacheTest.php @@ -203,6 +203,19 @@ public function testTagsCacheForever() $this->assertSame('bar', $store->tags($tags)->get('foo')); } + public function testFlushFunctionDoesntLeakMemory() + { + $store = new ArrayStore; + $tags = ['bop']; + $before = memory_get_usage(true); + for ($i = 0; $i < 100; $i++) { + $store->tags($tags)->forever('foo', 'bar'); + $store->tags($tags)->flush(); + } + $after = memory_get_usage(true); + $this->assertSame($before, $after); + } + private function getTestCacheStoreWithTagValues(): ArrayStore { $store = new ArrayStore; From e2cc24a6dfc9d485f7f7ba5d76bfd5126e15c6cf Mon Sep 17 00:00:00 2001 From: Matt Ford Date: Fri, 18 Aug 2023 09:30:16 +0100 Subject: [PATCH 5/8] Make the test more robust --- tests/Cache/CacheTaggedCacheTest.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/Cache/CacheTaggedCacheTest.php b/tests/Cache/CacheTaggedCacheTest.php index 7ee140c718e3..3744fc8d8ce1 100644 --- a/tests/Cache/CacheTaggedCacheTest.php +++ b/tests/Cache/CacheTaggedCacheTest.php @@ -208,8 +208,11 @@ public function testFlushFunctionDoesntLeakMemory() $store = new ArrayStore; $tags = ['bop']; $before = memory_get_usage(true); + // Store a 5MB cache value then flush it 100 times, then verify the overall memory usage did not increase for ($i = 0; $i < 100; $i++) { - $store->tags($tags)->forever('foo', 'bar'); + $key = str_replace('.', '', uniqid()); + $value = bin2hex(random_bytes(1024 * 5)); + $store->tags($tags)->forever($key, $value); $store->tags($tags)->flush(); } $after = memory_get_usage(true); From 83dbce167f07911232433a0bff0c6d8614049cdc Mon Sep 17 00:00:00 2001 From: Matt Ford Date: Fri, 18 Aug 2023 09:36:48 +0100 Subject: [PATCH 6/8] Style fix --- src/Illuminate/Cache/TaggedCache.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Illuminate/Cache/TaggedCache.php b/src/Illuminate/Cache/TaggedCache.php index 4f4463276c69..200af8e585da 100644 --- a/src/Illuminate/Cache/TaggedCache.php +++ b/src/Illuminate/Cache/TaggedCache.php @@ -126,7 +126,7 @@ protected function event($event) $itemKey = $this->itemKey($event->key); if ($itemKey !== $this->getMetadataKey() && ($event instanceof KeyWritten || $event instanceof KeyForgotten)) { $itemKeys = $this->getItemKeys(); - if ($event instanceof KeyWritten && !in_array($itemKey, $itemKeys)) { + if ($event instanceof KeyWritten && ! in_array($itemKey, $itemKeys)) { $itemKeys[] = $itemKey; } elseif ($event instanceof KeyForgotten && in_array($itemKey, $this->itemKeys)) { $itemKeys = array_values( From 6a94fdd3e0f48176bb7ba6018f51291d93e3a346 Mon Sep 17 00:00:00 2001 From: Matt Ford Date: Fri, 18 Aug 2023 10:02:27 +0100 Subject: [PATCH 7/8] Slight refactor to take the RedisTaggedCache into account --- src/Illuminate/Cache/RedisTaggedCache.php | 10 +++++ src/Illuminate/Cache/TaggedCache.php | 47 ++++++++++++++--------- 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/src/Illuminate/Cache/RedisTaggedCache.php b/src/Illuminate/Cache/RedisTaggedCache.php index b8120be95c03..d691484a795b 100644 --- a/src/Illuminate/Cache/RedisTaggedCache.php +++ b/src/Illuminate/Cache/RedisTaggedCache.php @@ -126,4 +126,14 @@ public function flushStale() return true; } + + protected function onKeyWritten(string $key): void + { + // No need to do anything as the Redis store manages the entry list internally. + } + + protected function onKeyForgotten(string $key): void + { + // No need to do anything as the Redis store manages the entry list internally. + } } diff --git a/src/Illuminate/Cache/TaggedCache.php b/src/Illuminate/Cache/TaggedCache.php index 200af8e585da..13c9afe950f1 100644 --- a/src/Illuminate/Cache/TaggedCache.php +++ b/src/Illuminate/Cache/TaggedCache.php @@ -19,13 +19,6 @@ class TaggedCache extends Repository */ protected $tags; - /** - * List of keys within this namespace. - * - * @var array - */ - protected $itemKeys = []; - /** * Create a new tagged cache instance. * @@ -124,25 +117,41 @@ public function taggedItemKey($key) protected function event($event) { $itemKey = $this->itemKey($event->key); - if ($itemKey !== $this->getMetadataKey() && ($event instanceof KeyWritten || $event instanceof KeyForgotten)) { - $itemKeys = $this->getItemKeys(); - if ($event instanceof KeyWritten && ! in_array($itemKey, $itemKeys)) { - $itemKeys[] = $itemKey; - } elseif ($event instanceof KeyForgotten && in_array($itemKey, $this->itemKeys)) { - $itemKeys = array_values( - array_filter($itemKeys, function ($k) use ($itemKey) { - return $k !== $itemKey; - }) - ); + if ($itemKey !== $this->getMetadataKey()) { + if ($event instanceof KeyWritten) { + $this->onKeyWritten($itemKey); + } elseif ($event instanceof KeyForgotten) { + $this->onKeyForgotten($itemKey); } - $this->putItemKeys($itemKeys); } parent::event($event->setTags($this->tags->getNames())); } + protected function onKeyWritten(string $key): void + { + $itemKeys = $this->getItemKeys(); + if (! in_array($key, $itemKeys)) { + $itemKeys[] = $key; + $this->putItemKeys($itemKeys); + } + } + + protected function onKeyForgotten(string $key): void + { + $itemKeys = $this->getItemKeys(); + if (in_array($key, $itemKeys)) { + $itemKeys = array_values( + array_filter($itemKeys, function ($k) use ($key) { + return $k !== $key; + }) + ); + $this->putItemKeys($itemKeys); + } + } + private function getMetadataKey(): string { - return $this->taggedItemKey('meta:keys'); + return $this->itemKey('meta:entries'); } private function getItemKeys(): array From 3c49ffb2d933b9a79995dde099e8e93ede76b309 Mon Sep 17 00:00:00 2001 From: Matt Ford Date: Fri, 18 Aug 2023 10:10:09 +0100 Subject: [PATCH 8/8] Add newline as required by styleci --- src/Illuminate/Cache/TaggedCache.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Illuminate/Cache/TaggedCache.php b/src/Illuminate/Cache/TaggedCache.php index 13c9afe950f1..03936db3d8b8 100644 --- a/src/Illuminate/Cache/TaggedCache.php +++ b/src/Illuminate/Cache/TaggedCache.php @@ -161,6 +161,7 @@ private function getItemKeys(): array if (! is_array($keys)) { $keys = []; } + return $keys; }