Added retry logic for all git operations. #136

dryganets · 2018-02-03T11:18:01Z

Motivation

In the complex CI environments, intermittent issues happen. In order to prevent build failures, it is good to give a server an opportunity to recover from high load spikes.

Test Plan

Put in the ~/.gitconfig following section:
[http]
proxy = http://192.168.1.101:8888
As you don't have a proxy running on the port you will get a timeout error from git.
You could disable the proxy by editing the config file if you need.

nzec

Travis is failing. Please check.

dryganets · 2018-02-06T08:46:16Z

not ok ENOENT: no such file or directory, open './no-npmignore/.npmignore'

I had a different ENOENT on npm install locally until I removed package-lock file.

I don't think this failure is related to my changes.

dryganets · 2018-02-06T09:37:11Z

#137

created to test the error.
Repo without any changes triggers the same error.

zkat

Hey! This is a great idea, and thank you for taking the time to put this together. I've made a few comments for changes I'd like to see, with apologies for a conflict that was introduced because of another PR that's now been merged.

I think this is a great idea, and I really appreciate this!

zkat · 2018-02-06T21:26:57Z

lib/util/git.js

+  'Operation timed out',
+  'Failed to connect to .* Timed out',
+  'Connection reset by peer',
+  'Couldn\'t resolve host'


We should not retry on resolve failure, because that tends to be a sign of an offline state, and unlikely to be fixed on a retry. See the errors make-fetch-happen retries on.

will remove, thank you.

why this code still in repository???

pacote/lib/util/git.js

Lines 33 to 35 in 425c58d

'Connection timed out',

'Operation timed out',

'Failed to connect to .* Timed out',

because this retry logic, it make npm install process will never end, when user can not access github by http/https/ssh protocol(like in Intranet CI environments).

if i dit not put -ddd arguments in command line, and i will never see what happend with npm.

$ node ./bin/npm-cli.js install -ddd github:hello/world npm info it worked if it ends with ok npm verb cli [ '/Users/xqin/.nvm/versions/node/v8.9.4/bin/node', npm verb cli '/Users/xqin/Desktop/Work/npm/bin/npm-cli.js', npm verb cli 'install', npm verb cli '-ddd', npm verb cli 'github:hello/world' ] npm info using npm@6.8.0 npm info using node@v8.9.4 npm verb npm-session 146e905747a05a8f npm sill install loadCurrentTree npm sill install readLocalPackageData npm sill pacote Retrying git command: ls-remote -h -t ssh://git@github.com/hello/world.git attempt # 2 npm sill pacote Retrying git command: ls-remote -h -t ssh://git@github.com/hello/world.git attempt # 3 npm sill pacote Retrying git command: ls-remote -h -t ssh://git@github.com/hello/world.git attempt # 4 npm sill pacote Retrying git command: ls-remote -h -t ssh://git@github.com/hello/world.git attempt # 5 npm sill pacote Retrying git command: ls-remote -h -t ssh://git@github.com/hello/world.git attempt # 6 npm sill pacote Retrying git command: ls-remote -h -t ssh://git@github.com/hello/world.git attempt # 7 npm sill pacote Retrying git command: ls-remote -h -t ssh://git@github.com/hello/world.git attempt # 8 npm sill pacote Retrying git command: ls-remote -h -t ssh://git@github.com/hello/world.git attempt # 9 npm sill pacote Retrying git command: ls-remote -h -t ssh://git@github.com/hello/world.git attempt # 10 ⸨░░░░░░░░░░░░░░░░░░⸩ ⠦ rollbackFailedOptional: sill pacote Retrying git command: ls-remote -h -t ssh://git@github.com/hello/world.git attempt # 10

@zkat @dryganets

zkat · 2018-02-06T21:27:59Z

lib/util/git.js

@@ -25,6 +26,23 @@ const GOOD_ENV_VARS = new Set([
  'GIT_SSL_NO_VERIFY'
 ])

+const GIT_TRANSIENT_ERRORS = [
+  'remote error: Internal Server Error',
+  'fatal: Couldn\'t find remote ref ',


I think a failure to find a remote ref is very unlikely to be fixed by an immediate retry -- even if there's a timing issue, it's unlikely to be fixed in such a small window, and this particular failure, I would expect, is almost always an error the user themself has to fix.

zkat · 2018-02-06T21:29:04Z

lib/util/git.js

-            }
-          })
+        if (type === 'tag') {
+          const match = ref.match(/v?(\d+\.\d+\.\d+)$/)


This line is conflicting with the version in latest -- could you pull in that patch here?

zkat · 2018-02-06T21:30:46Z

lib/util/git.js

-    return execFileAsync(gitPath, gitArgs, mkOpts(gitOpts, opts))
+    return promiseRetry((retry, number) => {
+      if (number != 1) {
+        opts.log.info('pacote', 'Retrying git command: ' + gitArgs.join(' ') + ' attempt # ' + number);


if you're gonna log this, I'd rather this be logged in verbose instead.

zkat · 2018-02-06T21:30:58Z

lib/util/git.js

-    return cp.spawn(gitPath, gitArgs, mkOpts(gitOpts, opts))
+    return promiseRetry((retry, number) => {
+      if (number != 1) {
+        opts.log.info('pacote', 'Retrying git command: ' + gitArgs.join(' ') + ' attempt # ' + number);


ditto, on verbose

zkat · 2018-02-06T21:37:22Z

lib/util/git.js

+const GIT_TRANSIENT_ERROR_RE = new RegExp(GIT_TRANSIENT_ERRORS)
+
+function shouldRetry(error) {
+  return GIT_TRANSIENT_ERROR_RE.test(error);


This ; made me realize the standard check was misconfigured -- it's now been fixed, and you'll need to fix that standard linter violations in this patch, too. Mostly, it's just removing the semicolons, I think. You'll get the errors now when you rebase onto latest.

In the complex CI environments, intermittent issues happen. In order to prevent build failures, it is good to give a server an opportunity to recover from high load spikes. Put in the ~/.gitconfig following section: [http] proxy = http://192.168.1.101:8888 As you don't have a proxy running on the port you will get a timeout error from git.

dryganets · 2018-02-06T22:12:32Z

As for another CI failure
you need to re-generate the package-lock.json to make it gone.

see:
npm/npm#17444

dryganets · 2018-02-07T23:23:23Z

@zkat I believe I have fixes for all comments you made.

dryganets · 2018-02-08T18:01:59Z

It might be worth to reconsider the log level from silly to warn.
As log won't be visible unless retry happens.
And if retry happens it is very important to know about it as it might bring some light on the configuration problems of the build server.

Normally on the user machine, there are no extra logs anyway.

zkat

LGTM! :)

nzec suggested changes Feb 6, 2018

View reviewed changes

zkat suggested changes Feb 6, 2018

View reviewed changes

dryganets force-pushed the sergeyd/git-retry-logic branch from 3ce3da4 to 023c063 Compare February 6, 2018 21:57

dryganets added 3 commits February 6, 2018 15:01

review fixes

1fc8250

logs are silly

e5d5ff9

lint fixes

6f8695a

added couple of real cases where retry helped to solve problem

8bdcd0b

zkat approved these changes Mar 8, 2018

View reviewed changes

zkat merged commit 425c58d into zkat:latest Mar 8, 2018

xqin mentioned this pull request Mar 14, 2019

fix(git): limit retry times, avoid unlimited retry make npm install process will never end #172

Merged

This was referenced Jun 18, 2019

npm install failing on github repository ehmicky/gulp-execa#2

Closed

npm install failing on github repository #176

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added retry logic for all git operations. #136

Added retry logic for all git operations. #136

dryganets commented Feb 3, 2018

nzec left a comment

dryganets commented Feb 6, 2018

dryganets commented Feb 6, 2018

zkat left a comment

zkat Feb 6, 2018

dryganets Feb 6, 2018

xqin Mar 12, 2019 •

edited

Loading

xqin Mar 12, 2019

xqin Mar 12, 2019

zkat Feb 6, 2018

dryganets Feb 6, 2018

zkat Feb 6, 2018

dryganets Feb 7, 2018

zkat Feb 6, 2018

zkat Feb 6, 2018

zkat Feb 6, 2018

dryganets commented Feb 6, 2018 •

edited

Loading

dryganets commented Feb 7, 2018

dryganets commented Feb 8, 2018

zkat left a comment

	'Connection timed out',
	'Operation timed out',
	'Failed to connect to .* Timed out',

Added retry logic for all git operations. #136

Added retry logic for all git operations. #136

Conversation

dryganets commented Feb 3, 2018

Motivation

Test Plan

nzec left a comment

Choose a reason for hiding this comment

dryganets commented Feb 6, 2018

dryganets commented Feb 6, 2018

zkat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xqin Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryganets commented Feb 6, 2018 • edited Loading

dryganets commented Feb 7, 2018

dryganets commented Feb 8, 2018

zkat left a comment

Choose a reason for hiding this comment

xqin Mar 12, 2019 •

edited

Loading

dryganets commented Feb 6, 2018 •

edited

Loading