Skip to content

Commit b39a47b

Browse files
spawniaspawnia
committed
Use atomic file writes for query cache
1 parent e89a2d2 commit b39a47b

28 files changed

+108
-83
lines changed

‎CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,12 @@ You can find and compare releases at the [GitHub release page](https://github.co
99

1010
## Unreleased
1111

12+
## v6.63.1
13+
14+
### Fixed
15+
16+
- Use atomic file writes for query cache
17+
1218
## v6.63.0
1319

1420
### Added

‎src/Cache/QueryCache.php‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use Illuminate\Contracts\Cache\Factory as CacheFactory;
1010
use Illuminate\Contracts\Cache\Repository as CacheRepository;
1111
use Illuminate\Filesystem\Filesystem;
12+
use Nuwave\Lighthouse\Support\Utils;
1213

1314
class QueryCache
1415
{
@@ -30,7 +31,7 @@ public function __construct(
3031

3132
$this->enable = (bool) $config['enable'];
3233
$this->mode = $config['mode'] ?? 'store';
33-
$this->opcachePath = $config['opcache_path'] ?? base_path('bootstrap/cache');
34+
$this->opcachePath = rtrim($config['opcache_path'] ?? base_path('bootstrap/cache'), '/');
3435
$this->store = $config['store'] ?? null;
3536
$this->ttl = $config['ttl'] ?? null;
3637
}
@@ -86,43 +87,43 @@ protected function fromStoreOrParse(string $hash, \Closure $parse): DocumentNode
8687
/** @param \Closure(): DocumentNode $parse */
8788
protected function fromOPcacheOrParse(string $hash, \Closure $parse): DocumentNode
8889
{
89-
$filePath = $this->opcacheFilePath($hash);
90+
$path = $this->opcacheFilePath($hash);
9091

91-
if ($this->filesystem->exists($filePath)) {
92-
return $this->requireOPcacheFile($filePath);
92+
if ($this->filesystem->exists($path)) {
93+
return $this->requireOPcacheFile($path);
9394
}
9495

9596
$query = $parse();
9697

9798
$contents = static::opcacheFileContents($query);
98-
$this->filesystem->put(path: $filePath, contents: $contents, lock: true);
99+
Utils::atomicPut(filesystem: $this->filesystem, path: $path, contents: $contents);
99100

100101
return $query;
101102
}
102103

103104
/** @param \Closure(): DocumentNode $parse */
104105
protected function fromHybridOrParse(string $hash, \Closure $parse): DocumentNode
105106
{
106-
$filePath = $this->opcacheFilePath($hash);
107+
$path = $this->opcacheFilePath($hash);
107108

108-
if ($this->filesystem->exists($filePath)) {
109-
return $this->requireOPcacheFile($filePath);
109+
if ($this->filesystem->exists($path)) {
110+
return $this->requireOPcacheFile($path);
110111
}
111112

112113
$store = $this->makeCacheStore();
113114

114115
$contents = $store->get(key: "lighthouse:query:{$hash}");
115116
if (is_string($contents)) {
116-
$this->filesystem->put(path: $filePath, contents: $contents, lock: true);
117+
Utils::atomicPut(filesystem: $this->filesystem, path: $path, contents: $contents);
117118

118-
return $this->requireOPcacheFile($filePath);
119+
return $this->requireOPcacheFile($path);
119120
}
120121

121122
$query = $parse();
122123

123124
$contents = static::opcacheFileContents($query);
124125
$store->put(key: "lighthouse:query:{$hash}", value: $contents, ttl: $this->ttl);
125-
$this->filesystem->put(path: $filePath, contents: $contents, lock: true);
126+
Utils::atomicPut(filesystem: $this->filesystem, path: $path, contents: $contents);
126127

127128
return $query;
128129
}
@@ -144,10 +145,10 @@ public static function opcacheFileContents(DocumentNode $query): string
144145
return "<?php return {$queryArrayString};";
145146
}
146147

147-
protected function requireOPcacheFile(string $filePath): DocumentNode
148+
protected function requireOPcacheFile(string $path): DocumentNode
148149
{
149-
$astArray = require $filePath;
150-
assert(is_array($astArray), 'The cache file is expected to return an array.');
150+
$astArray = require $path;
151+
assert(is_array($astArray), "The query cache file at {$path} is expected to return an array.");
151152

152153
$astInstance = AST::fromArray($astArray);
153154
assert($astInstance instanceof DocumentNode, 'The AST array is expected to convert to a DocumentNode.');

‎src/Console/PrintSchemaCommand.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,8 @@ public function handle(ASTCache $cache, FilesystemManager $filesystemManager, Sc
8484
return;
8585
}
8686

87-
$filesystemManager->disk($disk)->put($filename, $schemaString);
87+
$filesystem = $filesystemManager->disk($disk);
88+
$filesystem->put(path: $filename, contents: $schemaString);
8889
$this->info("Wrote schema to disk ({$disk}) as {$filename}.");
8990
} else {
9091
$this->info($schemaString);

‎src/Exceptions/InvalidSchemaCacheContentsException.php‎

Lines changed: 0 additions & 15 deletions
This file was deleted.

‎src/Schema/AST/ASTCache.php‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
use Illuminate\Contracts\Config\Repository as ConfigRepository;
66
use Illuminate\Filesystem\Filesystem;
7-
use Nuwave\Lighthouse\Exceptions\InvalidSchemaCacheContentsException;
7+
use Nuwave\Lighthouse\Support\Utils;
88

99
/**
1010
* @phpstan-type CacheConfig array{
@@ -48,13 +48,7 @@ public function set(DocumentAST $documentAST): void
4848
);
4949
$contents = /** @lang PHP */ "<?php return {$variable};";
5050

51-
// Since the schema cache can be very large, we write it to a temporary file first.
52-
// This avoids issues with the filesystem not being able to write large files atomically.
53-
// Then, we move the temporary file to the final location which is an atomic operation.
54-
$path = $this->path();
55-
$partialPath = "{$path}.partial";
56-
$this->filesystem->put(path: $partialPath, contents: $contents, lock: true);
57-
$this->filesystem->move(path: $partialPath, target: $path);
51+
Utils::atomicPut(filesystem: $this->filesystem, path: $this->path(), contents: $contents);
5852
}
5953

6054
public function clear(): void
@@ -67,14 +61,12 @@ public function fromCacheOrBuild(callable $build): DocumentAST
6761
{
6862
$path = $this->path();
6963
if ($this->filesystem->exists($path)) {
70-
$ast = require $path;
71-
if (! is_array($ast)) {
72-
throw new InvalidSchemaCacheContentsException($path, $ast);
73-
}
64+
$astArray = require $path;
65+
assert(is_array($astArray), "The schema cache file at {$path} is expected to return an array.");
7466

75-
/** @var SerializableDocumentAST $ast */
67+
/** @var SerializableDocumentAST $astArray */
7668

77-
return DocumentAST::fromArray($ast);
69+
return DocumentAST::fromArray($astArray);
7870
}
7971

8072
$documentAST = $build();

‎src/Support/Utils.php‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace Nuwave\Lighthouse\Support;
44

55
use Illuminate\Container\Container;
6+
use Illuminate\Filesystem\Filesystem;
67
use Illuminate\Support\Str;
78
use Nuwave\Lighthouse\Exceptions\DefinitionException;
89

@@ -200,4 +201,22 @@ public static function toEnumValueName(string $name): string
200201
? $name
201202
: "_{$name}";
202203
}
204+
205+
/**
206+
* Write the contents to a file atomically.
207+
*
208+
* This is done by writing to a temporary file first and then renaming it.
209+
*
210+
* It circumvents the following issues:
211+
* - large files not being written completely before being read
212+
* - partial writes from other processes while we expect to read what we wrote
213+
*/
214+
public static function atomicPut(Filesystem $filesystem, string $path, string $contents): void
215+
{
216+
$randomSuffix = bin2hex(random_bytes(6));
217+
$tempPath = "{$path}.{$randomSuffix}";
218+
219+
$filesystem->put(path: $tempPath, contents: $contents);
220+
$filesystem->move(path: $tempPath, target: $path);
221+
}
203222
}

‎tests/Integration/DefaultSchemaTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ public function testUsersByName(): void
154154
->assertJsonCount(0, 'data.users.data');
155155
}
156156

157-
protected function usersByName(string $name): TestResponse
157+
private function usersByName(string $name): TestResponse
158158
{
159159
return $this->graphQL(/** @lang GraphQL */ '
160160
query ($name: String!) {

‎tests/Integration/Execution/DataLoader/PaginatedRelationLoaderTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ static function (): void {},
189189
$this->assertCount($first, $firstTask->tags);
190190
}
191191

192-
protected function makePaginationArgs(int $first): PaginationArgs
192+
private function makePaginationArgs(int $first): PaginationArgs
193193
{
194194
$paginatorArgs = new PaginationArgs(1, $first, new PaginationType('SIMPLE'));
195195
$paginatorArgs->first = $first;

‎tests/Integration/Federation/FederationSchemaTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ public function testPaginationTypesAreMarkedAsSharableWhenUsingFederationV2(): v
173173
$this->assertStringContainsString('type SimplePaginatorInfo @shareable {', $sdl);
174174
}
175175

176-
protected function _serviceSdl(): string
176+
private function _serviceSdl(): string
177177
{
178178
$response = $this->graphQL(/** @lang GraphQL */ '
179179
{

‎tests/Integration/QueryCacheTest.php‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,4 +244,25 @@ public function testModeHybridWithPopulatedCacheStoreButMissingFile(): void
244244
$event->assertDispatchedTimes(CacheHit::class, 1);
245245
$event->assertDispatchedTimes(KeyWritten::class, 0);
246246
}
247+
248+
public function testInvalidQueryCacheContents(): void
249+
{
250+
$filesystem = Storage::fake();
251+
252+
$config = $this->app->make(ConfigRepository::class);
253+
$config->set('lighthouse.query_cache.enable', true);
254+
$config->set('lighthouse.query_cache.mode', 'opcache');
255+
$config->set('lighthouse.query_cache.opcache_path', $filesystem->path(''));
256+
257+
$fileName = 'lighthouse-query-ec859ac754fc185143d0daf8dcfa644b5e2271e219b9a8f80c3a6fdfb0ce67d0.php';
258+
$filesystem->put($fileName, '');
259+
260+
$this->expectException(\AssertionError::class);
261+
$this->expectExceptionMessage("The query cache file at {$filesystem->path($fileName)} is expected to return an array.");
262+
$this->graphQL(/** @lang GraphQL */ <<<'GRAPHQL'
263+
{
264+
foo
265+
}
266+
GRAPHQL);
267+
}
247268
}

0 commit comments

Comments
 (0)