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

Added exponential backoff when moving npm installation directory #331

Merged
merged 2 commits into from Apr 16, 2018

Conversation

jchitel
Copy link

@jchitel jchitel commented Jan 19, 2018

Fixes issue when installing Node version: 8.9.1+

I found, to much frustration, that the npm installation folder was not being moved from nvm/temp/nvm-npm/{npm-version} to nvm/{node-version}/node_modules/npm. No matter how many times I retried, it just wasn't working.

Nothing in the source code seemed to be revealing anything obvious, so I decided to fork the repo and debug it manually using VS Code's Go plugin. Somehow, magically, when I stepped through the program line by line, the move went just fine without issues. This led me to believe that the problem was time-based.

Sure enough, when I added a ten-second delay before moving the directory, it worked just fine. It seems that (for my system anyway) Windows can take a long time to give access to large directories after being unzipped. For Node versions <8 this may not have been a problem because NPM's dependencies may have been smaller. But for recent versions this just won't cut it.

In my testing, it seemed that anywhere between 5 and 10 seconds is required before the move will succeed. I added an exponential backoff that will:

  • First try right away
  • If the first attempt fails, wait 1 second
  • If the second attempt fails, wait 2 seconds
  • ..., wait 4 seconds
  • ..., wait 8 seconds
  • ..., wait 16 seconds
  • If it fails after waiting the total 31 seconds, then it will expose the error to the user because there is likely some other problem

I tested this several times and it has worked every time, so I am pretty confident in it.

This is my first time using Go, so I don't know if there is a better standard library API to implement this, but I gave it my best shot. Feel free to refactor it if you know of a better way.

Copy link

@b-dur b-dur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having the same problem on my windows. Did also build and run the nvm locally where it worked. Would be nice to have a fix for this.

Do you know what the error message is, when it fails renaming? Wondering if the error could be used to get a more pin pointed solution.
This PR is basically just handling all errors in the same way, i.e. using a backoff logic.

err = os.Rename(filepath.Join(tempDir, "nvm-npm", "npm-"+npmv), filepath.Join(env.root, "v"+version, "node_modules", "npm"))
if err == nil { break }
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some code in this section is repetitive and could be simplified to:

        // sometimes Windows can take some time to enable access to large amounts of files after unzip, use exponential backoff to wait until it is ready
        for _, i := range [5]int{1, 2, 4, 8, 16, 0} {
          err = os.Rename(filepath.Join(tempDir, "nvm-npm", "npm-"+npmv), filepath.Join(env.root, "v"+version, "node_modules", "npm"))
          if err == nil { break }
          time.Sleep(time.Duration(i)*time.Second)
        }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also consider refactoring this into a backoff function.
could look something like:

func backoff(fn func() error, maxDuration int) error {
	for delay := 1; ; delay = delay * 2 {
		err := fn()
		maxDuration -= delay
		if err == nil || maxDuration < delay {
			return err
		}
		time.Sleep(time.Duration(delay) * time.Second)
	}
}

Then the whole block be replaced with

npmSourcePath := filepath.Join(tempDir, "nvm-npm", "npm-"+npmv)
npmTargetPath := filepath.Join(env.root, "v"+version, "node_modules", "npm")
err = backoff(func () error { 
    return os.Rename(npmSourcePath, npmTargetPath)
}, 32)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured some simplification/refactoring could be done. I just learned enough Go to be able to implement this solution. I'd be happy to add your suggestions.

As to the specific error message, I don't quite remember at the moment, just that it was a FS permission error. I tried to investigate to see if there was a way to wait for Windows to make the files available, but I couldn't find one, and that subject goes well over my head. I figured a backoff would be the simplest solution, because either way the problem is that we need to wait.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm new to Go too, and think this is a great project to learn a new language.

In first iteration just getting a general fix, and then later maybe do a more fragmented error handling. If there ever should become necessary.

@coreybutler
Copy link
Owner

coreybutler commented Apr 6, 2018

@jchitel - great detective work!

Ultimately, I think you've discovered a problem with the Unzip function in https://github.com/coreybutler/nvm-windows/blob/master/src/nvm/file/file.go. I would prefer that method be fixed by leveraging the new https://golang.org/pkg/archive/zip, rather than waiting for Windows (which is unreliable).

That said, this issue has plagued this project for too long. So, I would accept this with a few minor changes. First, I think @b-dur brought up some good points, and I think those suggestions should be incorporated. Of course, if you want to take a shot at fixing the unzip method, that would be fine.... I'm not going to be picky though. Either way, the project will move forwards thanks to your contribution.

@b-dur
Copy link

b-dur commented Apr 6, 2018

This PR potentially fixes Issues #300, #302, #305, and #335

Copy link

@b-dur b-dur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should be fixing a critical bug many users are facing.
There is room for improvements on the code, but getting a fix out is highly necessary, and then a revisit can be done at a later point.

@b-dur b-dur merged commit 24c57b8 into coreybutler:master Apr 16, 2018
@b-dur
Copy link

b-dur commented Apr 16, 2018

Thanks @jchitel for this PR 👍

@jchitel
Copy link
Author

jchitel commented May 29, 2018

No problem! I actually didn't even see that this was merged until just now. When can we expect a new release? My team has been using a custom-built binary from my changes for several months.

@coreybutler
Copy link
Owner

I'm wrapping up a major contract project this week... I'll try to push a new release early next week.

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.

None yet

4 participants