Skip to content
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.

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

Fix possible race condition in the caching of request graph #9675

Merged
merged 4 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions packages/core/cache/src/FSCache.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,21 @@ export class FSCache implements Cache {
await Promise.all(writePromises);
}

async deleteLargeBlob(key: string): Promise<void> {
const deletePromises: Promise<void>[] = [];

let i = 0;
let filePath = this.#getFilePath(key, i);

while (await this.fs.exists(filePath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

/nit fs.exists (as opposed to existsSync) prior to performing a file operation is "deprecated" as the existence could change between the two operations.

It's an edge case, but you should probably use existsSync here. I believe rimraf itself will never "fail" if the file doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I'll fix it up in my next PR

deletePromises.push(this.fs.rimraf(filePath));
i += 1;
filePath = this.#getFilePath(key, i);
}

await Promise.all(deletePromises);
}

async get<T>(key: string): Promise<?T> {
try {
let data = await this.fs.readFile(this._getCachePath(key));
Expand Down
4 changes: 4 additions & 0 deletions packages/core/cache/src/IDBCache.browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@ export class IDBCache implements Cache {
return this.setBlob(key, contents);
}

async deleteLargeBlob(key: string): Promise<void> {
await (await this.store).delete(STORE_NAME, key);
}

refresh(): void {
// NOOP
}
Expand Down
4 changes: 4 additions & 0 deletions packages/core/cache/src/LMDBCache.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ export class LMDBCache implements Cache {
return this.fsCache.setLargeBlob(key, contents, options);
}

deleteLargeBlob(key: string): Promise<void> {
return this.fsCache.deleteLargeBlob(key);
}

refresh(): void {
// Reset the read transaction for the store. This guarantees that
// the next read will see the latest changes to the store.
Expand Down
41 changes: 17 additions & 24 deletions packages/core/core/src/RequestTracker.js
Original file line number Diff line number Diff line change
Expand Up @@ -1352,6 +1352,9 @@ export default class RequestTracker {

let serialisedGraph = this.graph.serialize();

// Delete an existing request graph cache, to prevent invalid states
await this.options.cache.deleteLargeBlob(requestGraphKey);

let total = 0;
const serialiseAndSet = async (
key: string,
Expand Down Expand Up @@ -1440,33 +1443,23 @@ export default class RequestTracker {
}
}

queue
.add(() =>
serialiseAndSet(requestGraphKey, {
...serialisedGraph,
nodes: undefined,
}),
)
.catch(() => {
// Handle promise rejection
});
try {
await queue.run();

let opts = getWatcherOptions(this.options);
let snapshotPath = path.join(this.options.cacheDir, snapshotKey + '.txt');
queue
.add(() =>
this.options.inputFS.writeSnapshot(
this.options.watchDir,
snapshotPath,
opts,
),
)
.catch(() => {
// Handle promise rejection
// Set the request graph after the queue is flushed to avoid writing an invalid state
await serialiseAndSet(requestGraphKey, {
...serialisedGraph,
nodes: undefined,
});

try {
await queue.run();
let opts = getWatcherOptions(this.options);
let snapshotPath = path.join(this.options.cacheDir, snapshotKey + '.txt');

await this.options.inputFS.writeSnapshot(
this.options.watchDir,
snapshotPath,
opts,
);
} catch (err) {
// If we have aborted, ignore the error and continue
if (!signal?.aborted) throw err;
Expand Down
1 change: 1 addition & 0 deletions packages/core/types-internal/src/Cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export interface Cache {
contents: Buffer | string,
options?: {|signal?: AbortSignal|},
): Promise<void>;
deleteLargeBlob(key: string): Promise<void>;
getBuffer(key: string): Promise<?Buffer>;
/**
* In a multi-threaded environment, where there are potentially multiple Cache
Expand Down
Loading