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.

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

win: serialize read/write/appendFile and rename #55

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

joaocgreis
Copy link

Ref: suggestion by @iarna in npm/npm#10940 (comment)

rename can be used to atomically overwrite files in UNIX, but fails in Windows when the destination is open. A file can be deleted while open but will only be deleted on disk after it is closed, so a new file cannot take its place right away. This change adds a queue, making rename mutually exclusive with read/write/appendFile.

This approach still has two known drawbacks:

  • Does not work for multiple processes.
  • The path passed as argument is used to identify the file, so if a relative path is used it must always be the same. This is enough to solve the npm issue~~, but I could add a realpath call~~.

State must be shared between polyfills.js and graceful-fs.js. I changed the interface of polyfills.js, but can change to something else if more appropriate. Perhaps moving rename to graceful-fs.js or file{Enqueue|Next} to exports of polyfills.js?

rename can be used to atomically overwrite files in UNIX, but fails
in Windows when the destination is open. A file can be deleted while
open but will only be deleted on disk after it is closed, so a new
file cannot take its place right away. This change adds a queue,
making rename mutually exclusive with read/write/appendFile.
@joaocgreis
Copy link
Author

Cannot use realpath to disambiguate the path, because it creates a handle and that conflicts with rename.

@iarna
Copy link

iarna commented Jan 21, 2016

I had been imagining something a little simpler, just a simple "retry n times" type thing, something like (untested code follows):

  var fs$rename = fs.rename
  fs.rename = rename
  function rename (from, to, cb) {
    var triesRemaining = 10
    return go$rename(from, to, cb)

    function go$rename () {
      return fs$rename(from, to, function (err) {
        if (triesRemaining-- && err && err.code === 'EPERM') {
          setTimeout(go$rename, 10)
        } else {
          if (typeof cb === 'function')
            cb.apply(this, arguments)
        }
      })
    }
  }

@joaocgreis
Copy link
Author

rename already retries for up to 1 second. I tried to remove that, and without it npm fails much faster. This completely solves the problem, albeit for this specific and very similar scenarios.

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

Successfully merging this pull request may close these issues.

2 participants