diff --git a/src/Assets/AssetContainer.php b/src/Assets/AssetContainer.php index d4bbbee5db..3566540f3f 100644 --- a/src/Assets/AssetContainer.php +++ b/src/Assets/AssetContainer.php @@ -27,7 +27,6 @@ use Statamic\Facades\Search; use Statamic\Facades\Stache; use Statamic\Facades\URL; -use Statamic\Statamic; use Statamic\Support\Arr; use Statamic\Support\Str; use Statamic\Support\Traits\FluentlyGetsAndSets; @@ -345,7 +344,7 @@ public function listContents() public function contents() { - return Blink::onceIf(! Statamic::isWorker(), 'asset-listing-cache-'.$this->handle(), function () { + return Blink::once('asset-listing-cache-'.$this->handle(), function () { return app(AssetContainerContents::class)->container($this); }); } diff --git a/src/Assets/AssetContainerContents.php b/src/Assets/AssetContainerContents.php index fa9b33e427..9c32f2defb 100644 --- a/src/Assets/AssetContainerContents.php +++ b/src/Assets/AssetContainerContents.php @@ -6,7 +6,6 @@ use Illuminate\Support\Facades\Cache; use League\Flysystem\DirectoryListing; use Statamic\Facades\Stache; -use Statamic\Statamic; use Statamic\Support\Str; class AssetContainerContents @@ -31,7 +30,7 @@ public function container($container) */ public function all() { - if ($this->files && ! Statamic::isWorker()) { + if ($this->files) { return $this->files; } @@ -209,7 +208,7 @@ public function metaFilesIn($folder, $recursive) public function filteredFilesIn($folder, $recursive) { - if (isset($this->filteredFiles[$key = $folder.($recursive ? '-recursive' : '')]) && ! Statamic::isWorker()) { + if (isset($this->filteredFiles[$key = $folder.($recursive ? '-recursive' : '')])) { return $this->filteredFiles[$key]; } @@ -239,7 +238,7 @@ public function filteredFilesIn($folder, $recursive) public function filteredDirectoriesIn($folder, $recursive) { - if (isset($this->filteredDirectories[$key = $folder.($recursive ? '-recursive' : '')]) && ! Statamic::isWorker()) { + if (isset($this->filteredDirectories[$key = $folder.($recursive ? '-recursive' : '')])) { return $this->filteredDirectories[$key]; } @@ -297,10 +296,6 @@ public function add($path) $files = $this->all()->put($path, $metadata); - if (Statamic::isWorker()) { - $this->cacheStore()->put($this->key(), $files, $this->ttl()); - } - $this->filteredFiles = null; $this->filteredDirectories = null; diff --git a/src/Stache/Indexes/Index.php b/src/Stache/Indexes/Index.php index 61182a2e00..7e437258e7 100644 --- a/src/Stache/Indexes/Index.php +++ b/src/Stache/Indexes/Index.php @@ -3,7 +3,6 @@ namespace Statamic\Stache\Indexes; use Statamic\Facades\Stache; -use Statamic\Statamic; abstract class Index { @@ -66,17 +65,12 @@ public function load() } $loadingKey = $this->store->key().'/'.$this->name; - $currentlyLoadingThis = in_array($loadingKey, static::$loadingStack); static::$loadingStack[] = $loadingKey; try { $this->loaded = true; - if (Statamic::isWorker() && ! $currentlyLoadingThis) { - $this->loaded = false; - } - debugbar()->addMessage("Loading index: {$loadingKey}", 'stache'); $this->items = Stache::cacheStore()->get($this->cacheKey()); @@ -163,6 +157,12 @@ public function clear() Stache::cacheStore()->forget($this->cacheKey()); } + public function resetMemoizedState() + { + $this->loaded = false; + $this->items = null; + } + /** @deprecated */ public static function currentlyLoading() { diff --git a/src/Stache/ServiceProvider.php b/src/Stache/ServiceProvider.php index 6981985207..b54b619e67 100644 --- a/src/Stache/ServiceProvider.php +++ b/src/Stache/ServiceProvider.php @@ -2,12 +2,15 @@ namespace Statamic\Stache; +use Illuminate\Queue\Events\JobProcessing; +use Illuminate\Support\Facades\Event; use Illuminate\Support\ServiceProvider as LaravelServiceProvider; use Statamic\Assets\QueryBuilder as AssetQueryBuilder; use Statamic\Facades\File; use Statamic\Facades\Site; use Statamic\Stache\Query\EntryQueryBuilder; use Statamic\Stache\Query\SubmissionQueryBuilder; +use Statamic\Statamic; use Symfony\Component\Lock\LockFactory; use Symfony\Component\Lock\Store\FlockStore; @@ -45,6 +48,8 @@ public function boot() $stache->sites(Site::all()->keys()->all()); $this->registerStores($stache); + + $this->resetMemoizedStateBetweenJobs($stache); } private function registerStores($stache) @@ -70,6 +75,19 @@ private function registerStores($stache) $stache->registerStores($stores->all()); } + private function resetMemoizedStateBetweenJobs($stache) + { + Event::listen(JobProcessing::class, function () use ($stache) { + if (! Statamic::isWorker()) { + return; + } + + $stache->stores()->each->resetMemoizedState(); + + $this->app->make('stache.indexes')->each->resetMemoizedState(); + }); + } + private function locks() { if (config('statamic.stache.lock.enabled', true)) { diff --git a/src/Stache/Stores/AggregateStore.php b/src/Stache/Stores/AggregateStore.php index 334c0b6383..df641e9618 100644 --- a/src/Stache/Stores/AggregateStore.php +++ b/src/Stache/Stores/AggregateStore.php @@ -87,6 +87,13 @@ public function warm() $this->discoverStores()->each->warm(); } + public function resetMemoizedState() + { + parent::resetMemoizedState(); + + $this->stores->each->resetMemoizedState(); + } + public function paths() { return $this->discoverStores()->flatMap(function ($store) { diff --git a/src/Stache/Stores/ContainerAssetsStore.php b/src/Stache/Stores/ContainerAssetsStore.php index bec0212f60..a8cc442410 100644 --- a/src/Stache/Stores/ContainerAssetsStore.php +++ b/src/Stache/Stores/ContainerAssetsStore.php @@ -4,7 +4,6 @@ use Statamic\Facades\AssetContainer; use Statamic\Facades\Stache; -use Statamic\Statamic; use Statamic\Support\Str; class ContainerAssetsStore extends ChildStore @@ -54,7 +53,7 @@ public function getItemsFromFiles() public function paths() { - if ($this->paths && ! Statamic::isWorker()) { + if ($this->paths) { return $this->paths; } diff --git a/src/Stache/Stores/Store.php b/src/Stache/Stores/Store.php index c220b679e1..b9b067a76a 100644 --- a/src/Stache/Stores/Store.php +++ b/src/Stache/Stores/Store.php @@ -9,7 +9,6 @@ use Statamic\Stache\Exceptions\DuplicateKeyException; use Statamic\Stache\Indexes; use Statamic\Stache\Indexes\Index; -use Statamic\Statamic; use Statamic\Support\Arr; use Statamic\Support\Str; @@ -299,7 +298,7 @@ public function paths() { $this->handleFileChanges(); - if ($this->paths && ! Statamic::isWorker()) { + if ($this->paths) { return $this->paths; } @@ -378,6 +377,11 @@ public function clearCachedPaths() Stache::cacheStore()->forget($this->pathsCacheKey()); } + public function resetMemoizedState() + { + $this->paths = null; + } + protected function pathsCacheKey() { return "stache::indexes::{$this->key()}::path"; diff --git a/tests/Assets/AssetContainerTest.php b/tests/Assets/AssetContainerTest.php index 955c04bdb0..2125696045 100644 --- a/tests/Assets/AssetContainerTest.php +++ b/tests/Assets/AssetContainerTest.php @@ -9,7 +9,7 @@ use Illuminate\Support\Collection; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Event; -use Illuminate\Support\Facades\Request; +use Illuminate\Support\Facades\Facade; use Illuminate\Support\Facades\Storage; use League\Flysystem\DirectoryAttributes; use League\Flysystem\DirectoryListing; @@ -33,7 +33,6 @@ use Statamic\Fields\Blueprint; use Statamic\Filesystem\Filesystem; use Statamic\Filesystem\FlysystemAdapter; -use Tests\Fakes\FakeArtisanRequest; use Tests\PreventSavingStacheItemsToDisk; use Tests\TestCase; @@ -581,7 +580,7 @@ public function it_gets_the_files_from_the_cache_only_once() } #[Test] - public function it_gets_the_files_from_the_cache_every_time_if_running_in_a_queue_worker() + public function it_still_only_gets_the_files_from_the_cache_once_per_job_if_running_in_a_queue_worker() { $cacheKey = 'asset-list-contents-test'; @@ -602,11 +601,17 @@ public function it_gets_the_files_from_the_cache_every_time_if_running_in_a_queu $container = (new AssetContainer)->handle('test')->disk('test'); - Request::swap(new FakeArtisanRequest('queue:listen')); - $expected = ['one.jpg', 'two.jpg']; $this->assertEquals($expected, $container->files()->all()); $this->assertEquals(1, $cacheHits); + $this->assertEquals($expected, $container->files()->all()); + $this->assertEquals(1, $cacheHits); + + // Laravel's real queue worker daemon loop clears resolved facade instances + // before every job it processes, which resets the Blink-backed + // AssetContainerContents instance behind the `contents()` call. + Facade::clearResolvedInstances(); + $this->assertEquals($expected, $container->files()->all()); $this->assertEquals(2, $cacheHits); } @@ -709,7 +714,7 @@ public function it_gets_the_folders_from_the_cache_and_blink_only_once() } #[Test] - public function it_gets_the_folders_from_the_cache_and_blink_every_time_if_running_in_a_queue_worker() + public function it_still_only_gets_the_folders_from_the_cache_and_blink_once_per_job_if_running_in_a_queue_worker() { $cacheKey = 'asset-list-contents-test'; @@ -729,17 +734,24 @@ public function it_gets_the_folders_from_the_cache_and_blink_every_time_if_runni $container = (new AssetContainer)->handle('test')->disk('test'); - Request::swap(new FakeArtisanRequest('queue:listen')); - $expected = ['one', 'two']; $this->assertEquals($expected, $container->folders()->all()); $this->assertEquals(1, $cacheHits); $this->assertEquals($expected, $container->folders()->all()); - $this->assertEquals(2, $cacheHits); + $this->assertEquals(1, $cacheHits); + // Still within the same job (resolved facade instances haven't been cleared), + // a freshly newed up container reuses the same Blink-cached contents. $anotherInstanceOfTheContainer = (new AssetContainer)->handle('test')->disk('test'); $this->assertEquals($expected, $anotherInstanceOfTheContainer->folders()->all()); - $this->assertEquals(3, $cacheHits); + $this->assertEquals(1, $cacheHits); + + // Laravel's real queue worker daemon loop clears resolved facade instances + // before every job it processes, which resets the Blink-backed contents cache. + Facade::clearResolvedInstances(); + + $this->assertEquals($expected, $container->folders()->all()); + $this->assertEquals(2, $cacheHits); } #[Test] @@ -754,11 +766,12 @@ public function it_does_not_leak_stale_contents_state_across_calls_when_running_ $container = (new AssetContainer)->handle('test')->disk('test'); - Request::swap(new FakeArtisanRequest('queue:work')); - // First job populates the instance's $metaFiles cache. metaFilesIn() has no - // isWorker() guard, so if contents() reused the same instance across jobs - // the filtered result would stick around and bleed into the next job. + // memoization guard of its own, so if contents() reused the same instance + // across jobs the filtered result would stick around and bleed into the + // next job. In real usage, Laravel's queue worker daemon loop clears + // resolved facade instances before every job it processes, which resets + // the Blink-backed instance behind `contents()` at each job boundary. $this->assertEquals( ['.meta/a.txt.yaml'], $container->contents()->metaFilesIn('/', true)->keys()->all() @@ -772,6 +785,10 @@ public function it_does_not_leak_stale_contents_state_across_calls_when_running_ '.meta/b.txt.yaml' => ['type' => 'file', 'path' => '.meta/b.txt.yaml', 'dirname' => '.meta'], ])); + // Simulate the job boundary: Laravel clears resolved facade instances + // before every job. + Facade::clearResolvedInstances(); + $this->assertEquals( ['.meta/a.txt.yaml', '.meta/b.txt.yaml'], $container->contents()->metaFilesIn('/', true)->keys()->sort()->values()->all() diff --git a/tests/Stache/ServiceProviderTest.php b/tests/Stache/ServiceProviderTest.php new file mode 100644 index 0000000000..f9c882732c --- /dev/null +++ b/tests/Stache/ServiceProviderTest.php @@ -0,0 +1,111 @@ +directory('/path/to/directory'); + Stache::registerStore($store); + + $pathsCacheKey = "stache::indexes::{$store->key()}::path"; + $idIndexCacheKey = "stache::indexes::{$store->key()}::id"; + + Cache::put($pathsCacheKey, ['foo', 'bar']); + Cache::put($idIndexCacheKey, ['alfa' => 'alfa.md']); + + $hits = collect(); + Event::listen(CacheHit::class, function ($event) use (&$hits) { + $hits->push($event->key); + }); + + Request::swap(new FakeArtisanRequest('queue:work')); + + // Memoize both the store's paths and one of its indexes. + $store->paths(); + $store->index('id'); + $store->paths(); + $store->index('id'); + $this->assertEquals(1, $hits->filter(fn ($key) => $key === $pathsCacheKey)->count()); + $this->assertEquals(1, $hits->filter(fn ($key) => $key === $idIndexCacheKey)->count()); + + // Simulate the worker's daemon loop moving on to its next job. This is what + // actually fires the `JobProcessing` listener registered by the Stache + // service provider, as opposed to calling `resetMemoizedState()` directly. + Event::dispatch(new JobProcessing('sync', $this->fakeJob())); + + // Both should be re-fetched from the cache store now that the job boundary has passed. + $store->paths(); + $store->index('id'); + $this->assertEquals(2, $hits->filter(fn ($key) => $key === $pathsCacheKey)->count()); + $this->assertEquals(2, $hits->filter(fn ($key) => $key === $idIndexCacheKey)->count()); + } + + #[Test] + public function it_does_not_reset_memoized_state_for_jobs_processed_outside_a_worker() + { + // `Illuminate\Queue\SyncQueue` also fires `JobProcessing` for jobs dispatched + // synchronously during an ordinary web request, so the listener must stay a + // no-op outside of an actual `queue:*`/`horizon:*` worker process. + $store = (new ServiceProviderTestStore)->directory('/path/to/directory'); + Stache::registerStore($store); + + $pathsCacheKey = "stache::indexes::{$store->key()}::path"; + + Cache::put($pathsCacheKey, ['foo', 'bar']); + + $hits = 0; + Event::listen(CacheHit::class, function ($event) use (&$hits, $pathsCacheKey) { + if ($event->key === $pathsCacheKey) { + $hits++; + } + }); + + $store->paths(); + $this->assertEquals(1, $hits); + + Event::dispatch(new JobProcessing('sync', $this->fakeJob())); + + $store->paths(); + $this->assertEquals(1, $hits); + } + + private function fakeJob(): Job + { + $job = Mockery::mock(Job::class); + $job->shouldReceive('payload')->andReturn([]); + + return $job; + } +} + +class ServiceProviderTestStore extends Store +{ + public function getItem($key) + { + } + + public function getItemValues($keys, $valueIndex, $keyIndex) + { + } + + public function key() + { + return 'service-provider-test-store'; + } +} diff --git a/tests/Stache/StoreTest.php b/tests/Stache/StoreTest.php index 419e2d2ddb..9e22f0494b 100644 --- a/tests/Stache/StoreTest.php +++ b/tests/Stache/StoreTest.php @@ -5,11 +5,9 @@ use Illuminate\Cache\Events\CacheHit; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Event; -use Illuminate\Support\Facades\Request; use PHPUnit\Framework\Attributes\Test; use Statamic\Stache\Stache; use Statamic\Stache\Stores\Store; -use Tests\Fakes\FakeArtisanRequest; use Tests\TestCase; class StoreTest extends TestCase @@ -64,7 +62,7 @@ public function it_gets_the_paths_from_the_cache_only_once() } #[Test] - public function it_gets_the_paths_from_the_cache_every_time_if_running_in_a_queue_worker() + public function it_still_only_gets_the_paths_from_the_cache_once_per_job_if_running_in_a_queue_worker() { $store = $this->store->directory('/path/to/directory'); $cacheKey = "stache::indexes::{$store->key()}::path"; @@ -78,11 +76,34 @@ public function it_gets_the_paths_from_the_cache_every_time_if_running_in_a_queu } }); - Request::swap(new FakeArtisanRequest('queue:listen')); + $expected = collect(['foo', 'bar']); + $this->assertEquals($expected, $store->paths()); + $this->assertEquals(1, $cacheHits); + $this->assertEquals($expected, $store->paths()); + $this->assertEquals(1, $cacheHits); + } + + #[Test] + public function it_gets_the_paths_from_the_cache_again_after_memoized_state_is_reset_between_jobs() + { + $store = $this->store->directory('/path/to/directory'); + $cacheKey = "stache::indexes::{$store->key()}::path"; + + Cache::put($cacheKey, ['foo', 'bar']); + + $cacheHits = 0; + Event::listen(CacheHit::class, function ($event) use (&$cacheHits, $cacheKey) { + if ($event->key === $cacheKey) { + $cacheHits++; + } + }); $expected = collect(['foo', 'bar']); $this->assertEquals($expected, $store->paths()); $this->assertEquals(1, $cacheHits); + + $store->resetMemoizedState(); + $this->assertEquals($expected, $store->paths()); $this->assertEquals(2, $cacheHits); }