From 7d54cbfaee041b2e6c4a79b34b2c943f33b22349 Mon Sep 17 00:00:00 2001 From: eric sciple Date: Wed, 4 Mar 2020 18:28:15 -0500 Subject: [PATCH] handle error response body error --- packages/artifact/package-lock.json | 5 - packages/tool-cache/RELEASES.md | 4 + .../tool-cache/__tests__/retry-helper.test.ts | 87 +++++++++++++ .../tool-cache/__tests__/tool-cache.test.ts | 102 +++++++++++++++ packages/tool-cache/package.json | 2 +- packages/tool-cache/src/retry-helper.ts | 55 +++++++++ packages/tool-cache/src/tool-cache.ts | 116 +++++++++++------- 7 files changed, 320 insertions(+), 51 deletions(-) create mode 100644 packages/tool-cache/__tests__/retry-helper.test.ts create mode 100644 packages/tool-cache/src/retry-helper.ts diff --git a/packages/artifact/package-lock.json b/packages/artifact/package-lock.json index d8cd2feb44..d4a64a1e36 100644 --- a/packages/artifact/package-lock.json +++ b/packages/artifact/package-lock.json @@ -4,11 +4,6 @@ "lockfileVersion": 1, "requires": true, "dependencies": { - "@actions/core": { - "version": "1.2.1", - "resolved": "https://registry.npmjs.org/@actions/core/-/core-1.2.1.tgz", - "integrity": "sha512-xD+CQd9p4lU7ZfRqmUcbJpqR+Ss51rJRVeXMyOLrZQImN9/8Sy/BEUBnHO/UKD3z03R686PVTLfEPmkropGuLw==" - }, "@actions/http-client": { "version": "1.0.6", "resolved": "https://registry.npmjs.org/@actions/http-client/-/http-client-1.0.6.tgz", diff --git a/packages/tool-cache/RELEASES.md b/packages/tool-cache/RELEASES.md index 24a6b70b5d..aeafeffdb9 100644 --- a/packages/tool-cache/RELEASES.md +++ b/packages/tool-cache/RELEASES.md @@ -1,5 +1,9 @@ # @actions/tool-cache Releases +### 1.3.2 + +- [Update downloadTool with better error handling and retries](https://github.com/actions/toolkit/pull/369) + ### 1.3.1 - [Increase http-client min version](https://github.com/actions/toolkit/pull/314) diff --git a/packages/tool-cache/__tests__/retry-helper.test.ts b/packages/tool-cache/__tests__/retry-helper.test.ts new file mode 100644 index 0000000000..cccf00757e --- /dev/null +++ b/packages/tool-cache/__tests__/retry-helper.test.ts @@ -0,0 +1,87 @@ +import * as core from '@actions/core' +import {RetryHelper} from '../src/retry-helper' + +let info: string[] +let retryHelper: RetryHelper + +describe('retry-helper tests', () => { + beforeAll(() => { + // Mock @actions/core info() + jest.spyOn(core, 'info').mockImplementation((message: string) => { + info.push(message) + }) + + retryHelper = new RetryHelper(3, 0, 0) + }) + + beforeEach(() => { + // Reset info + info = [] + }) + + afterAll(() => { + // Restore + jest.restoreAllMocks() + }) + + it('first attempt succeeds', async () => { + const actual = await retryHelper.execute(async () => { + return 'some result' + }) + expect(actual).toBe('some result') + expect(info).toHaveLength(0) + }) + + it('second attempt succeeds', async () => { + let attempts = 0 + const actual = await retryHelper.execute(async () => { + if (++attempts === 1) { + throw new Error('some error') + } + + return Promise.resolve('some result') + }) + expect(attempts).toBe(2) + expect(actual).toBe('some result') + expect(info).toHaveLength(2) + expect(info[0]).toBe('some error') + expect(info[1]).toMatch(/Waiting .+ seconds before trying again/) + }) + + it('third attempt succeeds', async () => { + let attempts = 0 + const actual = await retryHelper.execute(async () => { + if (++attempts < 3) { + throw new Error(`some error ${attempts}`) + } + + return Promise.resolve('some result') + }) + expect(attempts).toBe(3) + expect(actual).toBe('some result') + expect(info).toHaveLength(4) + expect(info[0]).toBe('some error 1') + expect(info[1]).toMatch(/Waiting .+ seconds before trying again/) + expect(info[2]).toBe('some error 2') + expect(info[3]).toMatch(/Waiting .+ seconds before trying again/) + }) + + it('all attempts fail succeeds', async () => { + let attempts = 0 + let error: Error = (null as unknown) as Error + try { + await retryHelper.execute(() => { + throw new Error(`some error ${++attempts}`) + }) + } catch (err) { + error = err + } + expect(error.message).toBe('some error 3') + expect(attempts).toBe(3) + expect(info).toHaveLength(4) + expect(info[0]).toBe('some error 1') + expect(info[1]).toMatch(/Waiting .+ seconds before trying again/) + expect(info[2]).toBe('some error 2') + expect(info[3]).toMatch(/Waiting .+ seconds before trying again/) + }) +}) diff --git a/packages/tool-cache/__tests__/tool-cache.test.ts b/packages/tool-cache/__tests__/tool-cache.test.ts index a28900856a..7888fa2adc 100644 --- a/packages/tool-cache/__tests__/tool-cache.test.ts +++ b/packages/tool-cache/__tests__/tool-cache.test.ts @@ -2,6 +2,7 @@ import * as fs from 'fs' import * as path from 'path' import * as io from '@actions/io' import * as exec from '@actions/exec' +import * as stream from 'stream' import nock from 'nock' const cachePath = path.join(__dirname, 'CACHE') @@ -24,6 +25,8 @@ describe('@actions/tool-cache', function() { username: 'abc', password: 'def' }) + setGlobal('TEST_DOWNLOAD_TOOL_RETRY_MIN_SECONDS', 0) + setGlobal('TEST_DOWNLOAD_TOOL_RETRY_MAX_SECONDS', 0) }) beforeEach(async function() { @@ -33,9 +36,15 @@ describe('@actions/tool-cache', function() { await io.mkdirP(tempPath) }) + afterEach(function() { + setResponseMessageFactory(undefined) + }) + afterAll(async function() { await io.rmRF(tempPath) await io.rmRF(cachePath) + setGlobal('TEST_DOWNLOAD_TOOL_RETRY_MIN_SECONDS', undefined) + setGlobal('TEST_DOWNLOAD_TOOL_RETRY_MAX_SECONDS', undefined) }) it('downloads a 35 byte file', async () => { @@ -90,6 +99,7 @@ describe('@actions/tool-cache', function() { it('downloads a 35 byte file after a redirect', async () => { nock('http://example.com') + .persist() .get('/redirect-to') .reply(303, undefined, { location: 'http://example.com/bytes/35' @@ -103,8 +113,75 @@ describe('@actions/tool-cache', function() { expect(fs.statSync(downPath).size).toBe(35) }) + it('handles error from response message stream', async () => { + nock('http://example.com') + .persist() + .get('/error-from-response-message-stream') + .reply(200, {}) + + setResponseMessageFactory(() => { + const readStream = new stream.Readable() + /* eslint-disable @typescript-eslint/unbound-method */ + readStream._read = () => { + readStream.destroy(new Error('uh oh')) + } + /* eslint-enable @typescript-eslint/unbound-method */ + return readStream + }) + + let error = new Error('unexpected') + try { + await tc.downloadTool( + 'http://example.com/error-from-response-message-stream' + ) + } catch (err) { + error = err + } + + expect(error).not.toBeUndefined() + expect(error.message).toBe('uh oh') + }) + + it('retries error from response message stream', async () => { + nock('http://example.com') + .persist() + .get('/retries-error-from-response-message-stream') + .reply(200, {}) + + /* eslint-disable @typescript-eslint/unbound-method */ + let attempt = 1 + setResponseMessageFactory(() => { + const readStream = new stream.Readable() + let failsafe = 0 + readStream._read = () => { + // First attempt fails + if (attempt++ === 1) { + readStream.destroy(new Error('uh oh')) + return + } + + // Write some data + if (failsafe++ === 0) { + readStream.push(''.padEnd(35)) + readStream.push(null) // no more data + } + } + + return readStream + }) + /* eslint-enable @typescript-eslint/unbound-method */ + + const downPath = await tc.downloadTool( + 'http://example.com/retries-error-from-response-message-stream' + ) + + expect(fs.existsSync(downPath)).toBeTruthy() + expect(fs.statSync(downPath).size).toBe(35) + }) + it('has status code in exception dictionary for HTTP error code responses', async () => { nock('http://example.com') + .persist() .get('/bytes/bad') .reply(400, { username: 'bad', @@ -124,6 +201,7 @@ describe('@actions/tool-cache', function() { it('works with redirect code 302', async function() { nock('http://example.com') + .persist() .get('/redirect-to') .reply(302, undefined, { location: 'http://example.com/bytes/35' @@ -542,3 +620,27 @@ describe('@actions/tool-cache', function() { } }) }) + +/** + * Sets up a mock response body for downloadTool. This function works around a limitation with + * nock when the response stream emits an error. + */ +function setResponseMessageFactory( + factory: (() => stream.Readable) | undefined +): void { + setGlobal('TEST_DOWNLOAD_TOOL_RESPONSE_MESSAGE_FACTORY', factory) +} + +/** + * Sets a global variable + */ +function setGlobal(key: string, value: T | undefined): void { + /* eslint-disable @typescript-eslint/no-explicit-any */ + const g = global as any + /* eslint-enable @typescript-eslint/no-explicit-any */ + if (value === undefined) { + delete g[key] + } else { + g[key] = value + } +} diff --git a/packages/tool-cache/package.json b/packages/tool-cache/package.json index 41f73293ef..7d2bf1e1b3 100644 --- a/packages/tool-cache/package.json +++ b/packages/tool-cache/package.json @@ -1,6 +1,6 @@ { "name": "@actions/tool-cache", - "version": "1.3.1", + "version": "1.3.2", "description": "Actions tool-cache lib", "keywords": [ "github", diff --git a/packages/tool-cache/src/retry-helper.ts b/packages/tool-cache/src/retry-helper.ts new file mode 100644 index 0000000000..36bde0bfed --- /dev/null +++ b/packages/tool-cache/src/retry-helper.ts @@ -0,0 +1,55 @@ +import * as core from '@actions/core' + +/** + * Internal class for retries + */ +export class RetryHelper { + private maxAttempts: number + private minSeconds: number + private maxSeconds: number + + constructor(maxAttempts: number, minSeconds: number, maxSeconds: number) { + if (maxAttempts < 1) { + throw new Error('max attempts should be greater than or equal to 1') + } + + this.maxAttempts = maxAttempts + this.minSeconds = Math.floor(minSeconds) + this.maxSeconds = Math.floor(maxSeconds) + if (this.minSeconds > this.maxSeconds) { + throw new Error('min seconds should be less than or equal to max seconds') + } + } + + async execute(action: () => Promise): Promise { + let attempt = 1 + while (attempt < this.maxAttempts) { + // Try + try { + return await action() + } catch (err) { + core.info(err.message) + } + + // Sleep + const seconds = this.getSleepAmount() + core.info(`Waiting ${seconds} seconds before trying again`) + await this.sleep(seconds) + attempt++ + } + + // Last attempt + return await action() + } + + private getSleepAmount(): number { + return ( + Math.floor(Math.random() * (this.maxSeconds - this.minSeconds + 1)) + + this.minSeconds + ) + } + + private async sleep(seconds: number): Promise { + return new Promise(resolve => setTimeout(resolve, seconds * 1000)) + } +} diff --git a/packages/tool-cache/src/tool-cache.ts b/packages/tool-cache/src/tool-cache.ts index 7cb581acd2..67cee42854 100644 --- a/packages/tool-cache/src/tool-cache.ts +++ b/packages/tool-cache/src/tool-cache.ts @@ -5,10 +5,13 @@ import * as os from 'os' import * as path from 'path' import * as httpm from '@actions/http-client' import * as semver from 'semver' +import * as stream from 'stream' +import * as util from 'util' import uuidV4 from 'uuid/v4' import {exec} from '@actions/exec/lib/exec' import {ExecOptions} from '@actions/exec/lib/interfaces' import {ok} from 'assert' +import {RetryHelper} from './retry-helper' export class HTTPError extends Error { constructor(readonly httpStatusCode: number | undefined) { @@ -55,55 +58,68 @@ export async function downloadTool( url: string, dest?: string ): Promise { - // Wrap in a promise so that we can resolve from within stream callbacks - return new Promise(async (resolve, reject) => { - try { - const http = new httpm.HttpClient(userAgent, [], { - allowRetries: true, - maxRetries: 3 - }) - dest = dest || path.join(tempDirectory, uuidV4()) - await io.mkdirP(path.dirname(dest)) - core.debug(`Downloading ${url}`) - core.debug(`Downloading ${dest}`) - - if (fs.existsSync(dest)) { - throw new Error(`Destination file path ${dest} already exists`) - } + dest = dest || path.join(tempDirectory, uuidV4()) + await io.mkdirP(path.dirname(dest)) + core.debug(`Downloading ${url}`) + core.debug(`Destination ${dest}`) + + const maxAttempts = 3 + const minSeconds = getGlobal( + 'TEST_DOWNLOAD_TOOL_RETRY_MIN_SECONDS', + 10 + ) + const maxSeconds = getGlobal( + 'TEST_DOWNLOAD_TOOL_RETRY_MAX_SECONDS', + 20 + ) + const retryHelper = new RetryHelper(maxAttempts, minSeconds, maxSeconds) + return await retryHelper.execute( + async () => await downloadToolAttempt(url, dest || '') + ) +} - const response: httpm.HttpClientResponse = await http.get(url) +async function downloadToolAttempt(url: string, dest: string): Promise { + if (fs.existsSync(dest)) { + throw new Error(`Destination file path ${dest} already exists`) + } - if (response.message.statusCode !== 200) { - const err = new HTTPError(response.message.statusCode) - core.debug( - `Failed to download from "${url}". Code(${response.message.statusCode}) Message(${response.message.statusMessage})` - ) - throw err - } + // Get the response headers + const http = new httpm.HttpClient(userAgent, [], { + allowRetries: false + }) + const response: httpm.HttpClientResponse = await http.get(url) + if (response.message.statusCode !== 200) { + const err = new HTTPError(response.message.statusCode) + core.debug( + `Failed to download from "${url}". Code(${response.message.statusCode}) Message(${response.message.statusMessage})` + ) + throw err + } - const file: NodeJS.WritableStream = fs.createWriteStream(dest) - file.on('open', async () => { - try { - const stream = response.message.pipe(file) - stream.on('close', () => { - core.debug('download complete') - resolve(dest) - }) - } catch (err) { - core.debug( - `Failed to download from "${url}". Code(${response.message.statusCode}) Message(${response.message.statusMessage})` - ) - reject(err) - } - }) - file.on('error', err => { - file.end() - reject(err) - }) - } catch (err) { - reject(err) + // Download the response body + const pipeline = util.promisify(stream.pipeline) + const responseMessageFactory = getGlobal<() => stream.Readable>( + 'TEST_DOWNLOAD_TOOL_RESPONSE_MESSAGE_FACTORY', + () => response.message + ) + const readStream = responseMessageFactory() + let succeeded = false + try { + await pipeline(readStream, fs.createWriteStream(dest)) + core.debug('download complete') + succeeded = true + return dest + } finally { + // Error, delete dest before retry + if (!succeeded) { + core.debug('download failed') + try { + await io.rmRF(dest) + } catch (err) { + core.debug(`Failed to delete '${dest}'. ${err.message}`) + } } - }) + } } /** @@ -516,3 +532,13 @@ function _evaluateVersions(versions: string[], versionSpec: string): string { return version } + +/** + * Gets a global variable + */ +function getGlobal(key: string, defaultValue: T): T { + /* eslint-disable @typescript-eslint/no-explicit-any */ + const value = (global as any)[key] as T | undefined + /* eslint-enable @typescript-eslint/no-explicit-any */ + return value !== undefined ? value : defaultValue +}