fix: eliminate race condition in cache for SHASUMS256

[erickzhao]

Fixes electron/packager#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.

[erikian]

nice

[MarshallOfSound]

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

We probably want to simply not put checksum files in the cache if we're never going to use them from the cache

[erickzhao]

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]

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]

Regardless, going to close this in favour of #267