Fix race condition reading existing import-map.json files #3453
Conversation
When trying to read the import-map.json, if that file is being concurrently using `fs.stat` won't tell you that. Using `fs.open` gives you a handle with `r+` (read and write) permission, which throws if it's open somewhere else for writing. If not, you get a handle and can read the data. Fixes withastro/astro#468
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/9BAGGsUu5aSpHAA4erUapgL34Ne5 |
|
Would it be worth looking at https://github.com/dubzzz/fast-check/blob/main/documentation/Tips.md#detect-race-conditions for being able to codify this as a test nonetheless? |
| const existingImportMap: ImportMap | null = | ||
| (await fs.stat(existingImportMapLoc).catch(() => null)) && | ||
| JSON.parse(await fs.readFile(existingImportMapLoc, 'utf8')); | ||
| const importMapHandle = await fs.open(existingImportMapLoc, 'r+').catch(() => null); |
There was a problem hiding this comment.
Like the PR notes said, it’s hard for me to test/confirm this but overall this seems like a pretty safe change to me, so I’m +1
|
Thanks for the suggestion @jasikpark . That seems to be its own testing framework so not something we can use here. But brings up the fact that it might be possible to test race conditions, so I'll look into libraries and see what I can find. |
It looks like it has support for Jest and Mocha integration, ~~so maybe it can support uvu? 🤔 ~~ Oh wait I just realized this is snowpack not astro - so then it should be usable since fast-check works out of the box with Jest! https://github.com/dubzzz/fast-check/blob/main/documentation/RaceConditions.md |
When trying to read the import-map.json, if that file is being concurrently written using
fs.statwon't tell you that. Usingfs.opengives you a handle withr+(read and write) permission, which throws if it's open somewhere else for writing. If not, you get a handle and can read the data.Fixes withastro/astro#468
Changes
Changes the logic in reading import-map.json to use a filehandle.
Testing
Did not, this would be hard to recreate because it's a race condition.
Docs
N/A