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.

Sign up for GitHub

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

Jump to bottom

Retry artifact download when response stream is truncated #652

Closed
wants to merge 4 commits into from
+204 −44
Closed

Retry artifact download when response stream is truncated #652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ccf139
Remove spyon, use readable stream to test pipeResponseToFile
hashtagchris Nov 26, 2020
d617c27
Check file contents
hashtagchris Nov 30, 2020
aa9ed1d
Retry artifact download when response stream is truncated
hashtagchris Nov 26, 2020
7ade2bb
More error handling
hashtagchris Nov 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
223 changes: 181 additions & 42 deletions packages/artifact/__tests__/download.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,12 @@ import {
ListArtifactsResponse,
QueryArtifactResponse
} from '../src/internal/contracts'
import * as stream from 'stream'
import {gzip} from 'zlib'
import {promisify} from 'util'

const root = path.join(__dirname, '_temp', 'artifact-download-tests')
const defaultEncoding = 'utf8'

jest.mock('../src/internal/config-variables')
jest.mock('@actions/http-client')
Expand Down Expand Up @@ -114,55 +118,122 @@ describe('Download Tests', () => {
})

it('Test downloading an individual artifact with gzip', async () => {
setupDownloadItemResponse(true, 200)
const fileContents = Buffer.from(
'gzip worked on the first try\n',
defaultEncoding
)
const targetPath = path.join(root, 'FileA.txt')

setupDownloadItemResponse(true, 200, false, fileContents)
const downloadHttpClient = new DownloadHttpClient()

const items: DownloadItem[] = []
items.push({
sourceLocation: `${configVariables.getRuntimeUrl()}_apis/resources/Containers/13?itemPath=my-artifact%2FFileA.txt`,
targetPath: path.join(root, 'FileA.txt')
targetPath
})

await expect(
downloadHttpClient.downloadSingleArtifact(items)
).resolves.not.toThrow()

await checkDestinationFile(targetPath, fileContents)
})

it('Test downloading an individual artifact without gzip', async () => {
setupDownloadItemResponse(false, 200)
const fileContents = Buffer.from(
'plaintext worked on the first try\n',
defaultEncoding
)
const targetPath = path.join(root, 'FileB.txt')

setupDownloadItemResponse(false, 200, false, fileContents)
const downloadHttpClient = new DownloadHttpClient()

const items: DownloadItem[] = []
items.push({
sourceLocation: `${configVariables.getRuntimeUrl()}_apis/resources/Containers/13?itemPath=my-artifact%2FFileB.txt`,
targetPath: path.join(root, 'FileB.txt')
targetPath
})

await expect(
downloadHttpClient.downloadSingleArtifact(items)
).resolves.not.toThrow()

await checkDestinationFile(targetPath, fileContents)
})

it('Test retryable status codes during artifact download', async () => {
// The first http response should return a retryable status call while the subsequent call should return a 200 so
// the download should successfully finish
const retryableStatusCodes = [429, 502, 503, 504]
for (const statusCode of retryableStatusCodes) {
setupDownloadItemResponse(false, statusCode)
const fileContents = Buffer.from('try, try again\n', defaultEncoding)
const targetPath = path.join(root, `FileC-${statusCode}.txt`)

setupDownloadItemResponse(false, statusCode, false, fileContents)
const downloadHttpClient = new DownloadHttpClient()

const items: DownloadItem[] = []
items.push({
sourceLocation: `${configVariables.getRuntimeUrl()}_apis/resources/Containers/13?itemPath=my-artifact%2FFileC.txt`,
targetPath: path.join(root, 'FileC.txt')
targetPath
})

await expect(
downloadHttpClient.downloadSingleArtifact(items)
).resolves.not.toThrow()

await checkDestinationFile(targetPath, fileContents)
}
})

it('Test retry on truncated response with gzip', async () => {
const fileContents = Buffer.from(
'Sometimes gunzip fails on the first try\n',
defaultEncoding
)
const targetPath = path.join(root, 'FileD.txt')

setupDownloadItemResponse(true, 200, true, fileContents)
const downloadHttpClient = new DownloadHttpClient()

const items: DownloadItem[] = []
items.push({
sourceLocation: `${configVariables.getRuntimeUrl()}_apis/resources/Containers/13?itemPath=my-artifact%2FFileD.txt`,
targetPath
})

await expect(
downloadHttpClient.downloadSingleArtifact(items)
).resolves.not.toThrow()

await checkDestinationFile(targetPath, fileContents)
})

it('Test retry on truncated response without gzip', async () => {
const fileContents = Buffer.from(
'You have to inspect the content-length header to know if you got everything\n',
defaultEncoding
)
const targetPath = path.join(root, 'FileE.txt')

setupDownloadItemResponse(false, 200, true, fileContents)
const downloadHttpClient = new DownloadHttpClient()

const items: DownloadItem[] = []
items.push({
sourceLocation: `${configVariables.getRuntimeUrl()}_apis/resources/Containers/13?itemPath=my-artifact%2FFileD.txt`,
targetPath
})

await expect(
downloadHttpClient.downloadSingleArtifact(items)
).resolves.not.toThrow()

await checkDestinationFile(targetPath, fileContents)
})

/**
* Helper used to setup mocking for the HttpClient
*/
Expand Down Expand Up @@ -227,51 +298,109 @@ describe('Download Tests', () => {
*/
function setupDownloadItemResponse(
isGzip: boolean,
firstHttpResponseCode: number
firstHttpResponseCode: number,
truncateFirstResponse: boolean,
fileContents: Buffer
): void {
jest
.spyOn(DownloadHttpClient.prototype, 'pipeResponseToFile')
.mockImplementationOnce(async () => {
return new Promise<void>(resolve => {
resolve()
})
})

jest
const spyInstance = jest
.spyOn(HttpClient.prototype, 'get')
.mockImplementationOnce(async () => {
const mockMessage = new http.IncomingMessage(new net.Socket())
mockMessage.statusCode = firstHttpResponseCode
if (isGzip) {
mockMessage.headers = {
'content-type': 'gzip'
if (firstHttpResponseCode === 200) {
const fullResponse = await constructResponse(isGzip, fileContents)
const actualResponse = truncateFirstResponse
? fullResponse.subarray(0, 3)
: fullResponse

return {
message: getDownloadResponseMessage(
firstHttpResponseCode,
isGzip,
fullResponse.length,
actualResponse
),
readBody: emptyMockReadBody
}
}

return new Promise<HttpClientResponse>(resolve => {
resolve({
message: mockMessage,
} else {
return {
message: getDownloadResponseMessage(
firstHttpResponseCode,
false,
0,
null
),
readBody: emptyMockReadBody
})
})
})
.mockImplementationOnce(async () => {
// chained response, if the HTTP GET function gets called again, return a successful response
const mockMessage = new http.IncomingMessage(new net.Socket())
mockMessage.statusCode = 200
if (isGzip) {
mockMessage.headers = {
'content-type': 'gzip'
}
}
})

return new Promise<HttpClientResponse>(resolve => {
resolve({
message: mockMessage,
readBody: emptyMockReadBody
})
})
// set up a second mock only if we expect a retry. Otherwise this mock will affect other tests.
if (firstHttpResponseCode !== 200) {
spyInstance.mockImplementationOnce(async () => {
// chained response, if the HTTP GET function gets called again, return a successful response
const fullResponse = await constructResponse(isGzip, fileContents)
return {
message: getDownloadResponseMessage(
200,
isGzip,
fullResponse.length,
fullResponse
),
readBody: emptyMockReadBody
}
})
}
}

async function constructResponse(
isGzip: boolean,
plaintext: Buffer | string
): Promise<Buffer> {
if (isGzip) {
return <Buffer>await promisify(gzip)(plaintext)
} else if (typeof plaintext === 'string') {
return Buffer.from(plaintext, defaultEncoding)
} else {
return plaintext
}
}

function getDownloadResponseMessage(
httpResponseCode: number,
isGzip: boolean,
contentLength: number,
response: Buffer | null
): http.IncomingMessage {
let readCallCount = 0
const mockMessage = <http.IncomingMessage>new stream.Readable({
read(size) {
switch (readCallCount++) {
case 0:
if (!!response && response.byteLength > size) {
throw new Error(
`test response larger than requested size (${size})`
)
}
this.push(response)
break

default:
// end the stream
this.push(null)
break
}
}
})

mockMessage.statusCode = httpResponseCode
mockMessage.headers = {
'content-length': contentLength.toString()
}

if (isGzip) {
mockMessage.headers['content-encoding'] = 'gzip'
}

return mockMessage
}

/**
Expand Down Expand Up @@ -347,4 +476,14 @@ describe('Download Tests', () => {
})
})
}

async function checkDestinationFile(
targetPath: string,
expectedContents: Buffer
): Promise<void> {
const fileContents = await fs.readFile(targetPath)

expect(fileContents.byteLength).toEqual(expectedContents.byteLength)
expect(fileContents.equals(expectedContents)).toBeTruthy()
}
})
25 changes: 23 additions & 2 deletions packages/artifact/src/internal/download-http-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,26 +263,47 @@ export class DownloadHttpClient {
if (isGzip) {
const gunzip = zlib.createGunzip()
response.message
.on('error', error => {
Copy link
Contributor Author

@hashtagchris hashtagchris Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The existing on('error', ...) callback below isn't called when gunzip fails, or response.message produces an error. pipe doesn't propagate errors across streams, so you have to monitor each. Without the new blocks the error is unhandled, and the new test times out.

I'm closing the downstream stream (hah!) on error. From https://nodejs.org/dist/latest-v14.x/docs/api/stream.html#stream_readable_pipe_destination_options:

One important caveat is that if the Readable stream emits an error during processing, the Writable destination is not closed automatically. If an error occurs, it will be necessary to manually close each stream in order to prevent memory leaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little bit unclear about when to retry here. If the download failed because the socket is opened too long (for downloading large files), would retry help?

Copy link
Contributor Author

@hashtagchris hashtagchris Nov 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good question. In short, I'm hoping retries will help. 😄

In the case of smaller files, I think there are likely transient http disconnects (timeouts?) that will benefit from retries.

For larger files (actions/download-artifact#55), is there a technical reason we'd be able to upload large files we can't download? Do we only chunk on upload? Should we add a max size check to upload-artifact if download-artifact is unlikely to succeed?

One option to add retries now, and then revisit this code if customers continue to report issues, say with larger files.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do chunked upload but I don't believe we do chunked download.

and then revisit this code if customers still report issues with larger files.

This is what I am afraid of, since in actions/download-artifact#55 the description says there is a 100% failure rate, I am afraid the retry will be in vain, and we will incur more minutes per build from those retries.

core.error(
`An error occurred while attempting to read the response stream`
)
reject(error)
gunzip.close()
})
.pipe(gunzip)
.on('error', error => {
core.error(
`An error occurred while attempting to decompress the response stream`
)
reject(error)
destinationStream.close()
})
.pipe(destinationStream)
.on('close', () => {
resolve()
})
.on('error', error => {
core.error(
`An error has been encountered while decompressing and writing a downloaded file to ${destinationStream.path}`
`An error occurred while writing a downloaded file to ${destinationStream.path}`
)
reject(error)
})
} else {
response.message
.on('error', error => {
core.error(
`An error occurred while attempting to read the response stream`
)
reject(error)
destinationStream.close()
})
.pipe(destinationStream)
.on('close', () => {
resolve()
})
.on('error', error => {
core.error(
`An error has been encountered while writing a downloaded file to ${destinationStream.path}`
`An error occurred while writing a downloaded file to ${destinationStream.path}`
)
reject(error)
})
Expand Down