electron / get

Download Electron release artifacts
https://npm.im/@electron/get
MIT License
337 stars 106 forks source link

fix: eliminate race condition in cache for SHASUMS256 #294

Closed erickzhao closed 1 month ago

erickzhao commented 1 month ago

Fixes https://github.com/electron/packager/issues/1552

When downloading the same version across multiple architectures at once (e.g. for a Universal macOS build), it's possible for fs.move to run into a race condition because the same SHASUMS256 file is being written to the cache for each arch.

@dsanders11 kindly pointed out that the code we have in putFileInCache is trying to mimic the overwrite option in fs.move, but follows an antipattern in Node.js:

Using fs.exists() to check for the existence of a file before calling fs.open(), fs.readFile(), or fs.writeFile() is not recommended. Doing so introduces a race condition, since other processes may change the file’s state between the two calls. Instead, user code should open/read/write the file directly and handle the error raised if the file does not exist.

erickzhao commented 1 month ago

I don't think this is semantically correct as it allows one download job to override the other downloads checksums in flight.

Is that creating a separate race condition where we might have a limbo state where we attempt to read from SHASUMS256 mid-overwrite? Both files are otherwise be 100% identical since this is an arch/platform-agnostic SHASUMS256.txt file.

dsanders11 commented 1 month ago

I don't think this is semantically correct as it allows one download job to override the other downloads checksums in flight.

Isn't that the current behavior? If the file already exists it removes it before putting the new one in the cache. It's effectively already an overwrite, just one that's prone to race conditions by checking first.

erickzhao commented 1 month ago

Regardless, going to close this in favour of #267