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.

Sign up for GitHub

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: use atomic writes for data store file operations #12715

Merged
merged 5 commits into from
Dec 13, 2024
+48 −5
Merged

fix: use atomic writes for data store file operations #12715

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8e9ecf0
fix: use atomic writes for data store file operations
ascorbic Dec 11, 2024
565f40f
Put tmp alongside the target
ascorbic Dec 11, 2024
de21fd3
Implement locking
ascorbic Dec 12, 2024
1402927
Refactor
ascorbic Dec 12, 2024
073ab1f
Wording
ascorbic Dec 12, 2024
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
5 changes: 5 additions & 0 deletions .changeset/tame-bags-remember.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes a bug that caused errors in dev when editing sites with large numbers of MDX pages
24 changes: 19 additions & 5 deletions packages/astro/src/content/mutable-data-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,23 @@ import { AstroError, AstroErrorData } from '../core/errors/index.js';
import { IMAGE_IMPORT_PREFIX } from './consts.js';
import { type DataEntry, ImmutableDataStore } from './data-store.js';
import { contentModuleToId } from './utils.js';
import { tmpdir } from 'node:os';
import { join } from 'node:path';

const SAVE_DEBOUNCE_MS = 500;

// Write a file atomically, without triggering the file watcher
async function writeFileAtomic(filePath: PathLike, data: string) {
const dir = await fs.mkdtemp(tmpdir() + '/astro-');
const tempFile = join(dir, 'temp');
await fs.writeFile(tempFile, data);
try {
await fs.rename(tempFile, filePath);
} finally {
await fs.rmdir(dir).catch(() => {});
}
Copy link
Member

@ematipico ematipico Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not too fond of this solution because now ALL projects, without discrimination, will incur in four (create temp dir, write file, rename file, remove directory) writing I/O operations instead of one. And writing operations are slow.

Maybe we could have an internal heuristic, where we enable this atomic rewriting only when we exceed a certain number of collections.

As you said, this is hard to test and incur in a project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a high-volume write though. It's debounced, so only called once in builds. Switching based on collection size is a good idea though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done a new approach which is just a single write and a rename. Renames are cheap, so it shouldn't be much overhead compared to the current approach.

}

/**
* Extends the DataStore with the ability to change entries and write them to disk.
* This is kept as a separate class to avoid needing node builtins at runtime, when read-only access is all that is needed.
Expand Down Expand Up @@ -86,7 +100,7 @@ export class MutableDataStore extends ImmutableDataStore {

if (this.#assetImports.size === 0) {
try {
await fs.writeFile(filePath, 'export default new Map();');
await writeFileAtomic(filePath, 'export default new Map();');
} catch (err) {
throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err });
}
Expand All @@ -110,7 +124,7 @@ ${imports.join('\n')}
export default new Map([${exports.join(', ')}]);
`;
try {
await fs.writeFile(filePath, code);
await writeFileAtomic(filePath, code);
} catch (err) {
throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err });
}
Expand All @@ -122,7 +136,7 @@ export default new Map([${exports.join(', ')}]);

if (this.#moduleImports.size === 0) {
try {
await fs.writeFile(filePath, 'export default new Map();');
await writeFileAtomic(filePath, 'export default new Map();');
} catch (err) {
throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err });
}
Expand All @@ -143,7 +157,7 @@ export default new Map([${exports.join(', ')}]);
export default new Map([\n${lines.join(',\n')}]);
`;
try {
await fs.writeFile(filePath, code);
await writeFileAtomic(filePath, code);
} catch (err) {
throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err });
}
Expand Down Expand Up @@ -298,7 +312,7 @@ export default new Map([\n${lines.join(',\n')}]);
return;
}
try {
await fs.writeFile(filePath, this.toString());
await writeFileAtomic(filePath, this.toString());
this.#file = filePath;
this.#dirty = false;
} catch (err) {
Expand Down
Loading