Skip to content

Commit

Permalink
update downloadTool to handle errors from response stream and retry
Browse files Browse the repository at this point in the history
  • Loading branch information
ericsciple committed Mar 5, 2020
1 parent 6459481 commit eab6335
Show file tree
Hide file tree
Showing 7 changed files with 320 additions and 51 deletions.
5 changes: 0 additions & 5 deletions packages/artifact/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions packages/tool-cache/RELEASES.md
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
87 changes: 87 additions & 0 deletions packages/tool-cache/__tests__/retry-helper.test.ts
Original file line number Diff line number Diff line change
@@ -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/)
})
})
102 changes: 102 additions & 0 deletions packages/tool-cache/__tests__/tool-cache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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() {
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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'
Expand All @@ -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',
Expand All @@ -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'
Expand Down Expand Up @@ -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<T>(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
}
}
2 changes: 1 addition & 1 deletion packages/tool-cache/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@actions/tool-cache",
"version": "1.3.1",
"version": "1.3.2",
"description": "Actions tool-cache lib",
"keywords": [
"github",
Expand Down
55 changes: 55 additions & 0 deletions packages/tool-cache/src/retry-helper.ts
Original file line number Diff line number Diff line change
@@ -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<T>(action: () => Promise<T>): Promise<T> {
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<void> {
return new Promise(resolve => setTimeout(resolve, seconds * 1000))
}
}
Loading

0 comments on commit eab6335

Please sign in to comment.