From 766de2f1913ab4a22cc9d419639e8e5eb7e7ba60 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Fri, 18 Jun 2021 09:17:28 -0700 Subject: [PATCH] Throw custom errors from spawn (#32) Ref: npm/cli#3149 --- lib/errors.js | 36 +++++++++++++++++++++++++ lib/index.js | 3 ++- lib/make-error.js | 33 +++++++++++++++++++++++ lib/should-retry.js | 17 ------------ lib/spawn.js | 9 ++++--- test/index.js | 7 ++++- test/make-error.js | 30 +++++++++++++++++++++ test/should-retry.js | 6 ----- test/spawn.js | 62 ++++++++++++++++++++++++++++++++++++++++---- 9 files changed, 169 insertions(+), 34 deletions(-) create mode 100644 lib/errors.js create mode 100644 lib/make-error.js delete mode 100644 lib/should-retry.js create mode 100644 test/make-error.js delete mode 100644 test/should-retry.js diff --git a/lib/errors.js b/lib/errors.js new file mode 100644 index 0000000..25b2b9f --- /dev/null +++ b/lib/errors.js @@ -0,0 +1,36 @@ + +const maxRetry = 3 + +class GitError extends Error { + shouldRetry () { + return false + } +} + +class GitConnectionError extends GitError { + constructor (message) { + super('A git connection error occurred') + } + + shouldRetry (number) { + return number < maxRetry + } +} + +class GitPathspecError extends GitError { + constructor (message) { + super('The git reference could not be found') + } +} + +class GitUnknownError extends GitError { + constructor (message) { + super('An unknown git error occurred') + } +} + +module.exports = { + GitConnectionError, + GitPathspecError, + GitUnknownError +} diff --git a/lib/index.js b/lib/index.js index 50fd889..20d7cfd 100644 --- a/lib/index.js +++ b/lib/index.js @@ -4,5 +4,6 @@ module.exports = { spawn: require('./spawn.js'), is: require('./is.js'), find: require('./find.js'), - isClean: require('./is-clean.js') + isClean: require('./is-clean.js'), + errors: require('./errors.js') } diff --git a/lib/make-error.js b/lib/make-error.js new file mode 100644 index 0000000..043a8e6 --- /dev/null +++ b/lib/make-error.js @@ -0,0 +1,33 @@ +const { + GitConnectionError, + GitPathspecError, + GitUnknownError +} = require('./errors.js') + +const connectionErrorRe = new RegExp([ + 'remote error: Internal Server Error', + 'The remote end hung up unexpectedly', + 'Connection timed out', + 'Operation timed out', + 'Failed to connect to .* Timed out', + 'Connection reset by peer', + 'SSL_ERROR_SYSCALL', + 'The requested URL returned error: 503' +].join('|')) + +const missingPathspecRe = /pathspec .* did not match any file\(s\) known to git/ + +function makeError (er) { + const message = er.stderr + let gitEr + if (connectionErrorRe.test(message)) { + gitEr = new GitConnectionError(message) + } else if (missingPathspecRe.test(message)) { + gitEr = new GitPathspecError(message) + } else { + gitEr = new GitUnknownError(message) + } + return Object.assign(gitEr, er) +} + +module.exports = makeError diff --git a/lib/should-retry.js b/lib/should-retry.js deleted file mode 100644 index 8082bb5..0000000 --- a/lib/should-retry.js +++ /dev/null @@ -1,17 +0,0 @@ -const transientErrors = [ - 'remote error: Internal Server Error', - 'The remote end hung up unexpectedly', - 'Connection timed out', - 'Operation timed out', - 'Failed to connect to .* Timed out', - 'Connection reset by peer', - 'SSL_ERROR_SYSCALL', - 'The requested URL returned error: 503' -].join('|') - -const transientErrorRe = new RegExp(transientErrors) - -const maxRetry = 3 - -module.exports = (error, number) => - transientErrorRe.test(error) && (number < maxRetry) diff --git a/lib/spawn.js b/lib/spawn.js index 337164a..1c89a4c 100644 --- a/lib/spawn.js +++ b/lib/spawn.js @@ -1,6 +1,6 @@ const spawn = require('@npmcli/promise-spawn') const promiseRetry = require('promise-retry') -const shouldRetry = require('./should-retry.js') +const makeError = require('./make-error.js') const whichGit = require('./which.js') const makeOpts = require('./opts.js') const procLog = require('./proc-log.js') @@ -33,10 +33,11 @@ module.exports = (gitArgs, opts = {}) => { return spawn(gitPath, args, makeOpts(opts)) .catch(er => { - if (!shouldRetry(er.stderr, number)) { - throw er + const gitError = makeError(er) + if (!gitError.shouldRetry(number)) { + throw gitError } - retry(er) + retry(gitError) }) }, retry) } diff --git a/test/index.js b/test/index.js index 39d2635..a8e11e1 100644 --- a/test/index.js +++ b/test/index.js @@ -3,5 +3,10 @@ const t = require('tap') t.match(git, { clone: Function, revs: Function, - spawn: Function + spawn: Function, + errors: { + GitConnectionError: /GitConnectionError/, + GitPathspecError: /GitPathspecError/, + GitUnknownError: /GitUnknownError/ + } }) diff --git a/test/make-error.js b/test/make-error.js new file mode 100644 index 0000000..f1e3653 --- /dev/null +++ b/test/make-error.js @@ -0,0 +1,30 @@ +const _makeError = require('../lib/make-error.js') +const errors = require('../lib/errors.js') +const t = require('tap') + +const makeError = (message) => + // Create the error with properties like it came from promise-spawn + _makeError(Object.assign(new Error(), { stderr: message })) + +t.test('throw matching error for missing pathspec', (t) => { + const missingPathspec = makeError('error: pathspec \'foo\' did not match any file(s) known to git') + t.ok(missingPathspec instanceof errors.GitPathspecError, 'error is a pathspec error') + + t.end() +}) + +t.test('only transient connection errors are retried', (t) => { + const sslError = makeError('SSL_ERROR_SYSCALL') + t.ok(sslError.shouldRetry(1), 'transient error, not beyond max') + t.ok(sslError instanceof errors.GitConnectionError, 'error is a connection error') + + const unknownError = makeError('asdf') + t.notOk(unknownError.shouldRetry(1), 'unknown error, do not retry') + t.ok(unknownError instanceof errors.GitUnknownError, 'error is an unknown error') + + const connectError = makeError('Failed to connect to fooblz Timed out') + t.notOk(connectError.shouldRetry(69), 'beyond max retries, do not retry') + t.ok(connectError instanceof errors.GitConnectionError, 'error is a connection error') + + t.end() +}) diff --git a/test/should-retry.js b/test/should-retry.js deleted file mode 100644 index 7d7ef99..0000000 --- a/test/should-retry.js +++ /dev/null @@ -1,6 +0,0 @@ -const shouldRetry = require('../lib/should-retry.js') -const t = require('tap') -t.ok(shouldRetry('SSL_ERROR_SYSCALL', 1), 'transient error, not beyond max') -t.notOk(shouldRetry('asdf', 1), 'unknown error, do not retry') -t.notOk(shouldRetry('Failed to connect to fooblz Timed out', 69), - 'beyond max retries, do not retry') diff --git a/test/spawn.js b/test/spawn.js index 9fe03b6..96a618f 100644 --- a/test/spawn.js +++ b/test/spawn.js @@ -1,5 +1,6 @@ const spawn = require('../lib/spawn.js') const procLog = require('../lib/proc-log.js') +const errors = require('../lib/errors.js') const t = require('tap') t.rejects(spawn(['status'], { git: false }), { @@ -44,9 +45,10 @@ t.test('argument test for allowReplace', async t => { t.test('retries', t => { const logs = [] process.on('log', (...log) => logs.push(log)) + const gitMessage = 'Connection timed out\n' const te = resolve(repo, 'transient-error.js') fs.writeFileSync(te, ` -console.error('Connection timed out') +console.error('${gitMessage.trim()}') process.exit(1) `) const retryOptions = { @@ -65,15 +67,15 @@ process.exit(1) fetchRetryMintimeout: 1 } } - const er = { - message: 'command failed', + const er = Object.assign(new errors.GitConnectionError(gitMessage), { cmd: process.execPath, args: [te], code: 1, signal: null, stdout: '', - stderr: 'Connection timed out\n' - } + stderr: gitMessage, + message: 'A git connection error occurred' + }) Object.keys(retryOptions).forEach(n => t.test(n, t => t.rejects(spawn([te], { cwd: repo, @@ -98,3 +100,53 @@ process.exit(1) }))) t.end() }) + +t.test('missing pathspec', t => { + const gitMessage = 'error: pathspec \'foo\' did not match any file(s) known to git\n' + const te = resolve(repo, 'pathspec-error.js') + fs.writeFileSync(te, ` +console.error("${gitMessage.trim()}") +process.exit(1) + `) + const er = Object.assign(new errors.GitPathspecError(gitMessage), { + cmd: process.execPath, + args: [te], + code: 1, + signal: null, + stdout: '', + stderr: gitMessage, + message: 'The git reference could not be found' + }) + t.rejects(spawn([te], { + cwd: repo, + git: process.execPath, + allowReplace: true, + log: procLog + }), er) + t.end() +}) + +t.test('unknown git error', t => { + const gitMessage = 'error: something really bad happened to git\n' + const te = resolve(repo, 'unknown-error.js') + fs.writeFileSync(te, ` +console.error("${gitMessage.trim()}") +process.exit(1) + `) + const er = Object.assign(new errors.GitUnknownError(gitMessage), { + cmd: process.execPath, + args: [te], + code: 1, + signal: null, + stdout: '', + stderr: gitMessage, + message: 'An unknown git error occurred' + }) + t.rejects(spawn([te], { + cwd: repo, + git: process.execPath, + allowReplace: true, + log: procLog + }), er) + t.end() +})