Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

cache: ignore failures replacing package.json #10940

Closed

Conversation

orangemocha
Copy link
Contributor

As reported in #9696 , npm install fails frequently on slow Windows machines.

This happens because npm uses write-file-atomic to overwrite package.json in the cache, and it is not possible to write atomically to a file in Windows as it is done in UNIX. To replace a file, it cannot be open at the same time, either for reading or for other calls to write-file-atomic.

Failures writing to package.json are already ignored in another place. Not saving cache information here does not seem to be an issue, but can there be a reason why we might not want to do this? @othiym23

Fixes: #7885
Fixes: #9696

writeFileAtomic is not atomic in Windows, it fails if the file is
being accessed concurrently.

Fixes: npm#7885
Fixes: npm#9696
@iarna
Copy link
Contributor

iarna commented Jan 6, 2016

I wonder if, instead of just ignoring this, that write-file-atomic should retry for a little bit if it gets EPERM errors during rename (in the same sort of way that graceful-fs provides for other fs calls). Or maybe we should just add rename to graceful-fs (which write-file-atomic already takes advantage of).

@othiym23
Copy link
Contributor

Alexis, could you rebase this against the latest 2.x head so we can see how it's doing with CI? Thanks! (Also, sorry for letting this languish for a bit. We've been busy!)

@joaocgreis
Copy link
Contributor

@othiym23 last commit on 2.x was on December 11, so this is still up to date.

As suggested by @iarna, submitted a PR to graceful-fs: isaacs/node-graceful-fs#55 . That should solve #9696 , but not when multiple npm instances run simultaneously as in #7885 . So this should still land (unless saving the cache here is essential for some reason).

@othiym23
Copy link
Contributor

Hmm... I guess the changes @zkat has made to get the tests passing for 2.x haven't been merged to the branch yet; I'd like to wait for a passing build before we merge this, as this is a significant change. That should happen as part of next week's release, so we'll circle back to this after that's gone out.

zkat pushed a commit that referenced this pull request Jan 19, 2016
writeFileAtomic is not atomic in Windows, it fails if the file is
being accessed concurrently.

Fixes: #7885
Fixes: #9696
PR-URL: #10940
Credit: @orangemocha
@iarna iarna added this to the next milestone Jan 20, 2016
iarna pushed a commit that referenced this pull request Jan 20, 2016
writeFileAtomic is not atomic in Windows, it fails if the file is
being accessed concurrently.

Fixes: #7885
Fixes: #9696
PR-URL: #10940
Credit: @orangemocha
Reviewed-By: @othiym23
iarna pushed a commit that referenced this pull request Jan 21, 2016
writeFileAtomic is not atomic in Windows, it fails if the file is
being accessed concurrently.

Fixes: #7885
Fixes: #9696
PR-URL: #10940
Credit: @orangemocha
Reviewed-By: @othiym23
@iarna
Copy link
Contributor

iarna commented Jan 25, 2016

This was merged to 3.6.0 & 2.14.16! =)

@iarna iarna closed this Jan 25, 2016
@orangemocha
Copy link
Contributor Author

Awesome, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants