From 520206f818a347616533bee5d4c0b226ee097ebf Mon Sep 17 00:00:00 2001 From: Chris Sidi Date: Wed, 25 Nov 2020 23:04:07 -0500 Subject: [PATCH 01/11] Retry artifact download when response stream is truncated --- packages/artifact/__tests__/download.test.ts | 71 +++++++++++++++++-- .../src/internal/download-http-client.ts | 6 ++ 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts index 305bfa8a..81759d46 100644 --- a/packages/artifact/__tests__/download.test.ts +++ b/packages/artifact/__tests__/download.test.ts @@ -124,7 +124,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileA.txt') - setupDownloadItemResponse(true, 200, fileContents) + setupDownloadItemResponse(true, 200, false, fileContents) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -147,7 +147,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileB.txt') - setupDownloadItemResponse(false, 200, fileContents) + setupDownloadItemResponse(false, 200, false, fileContents) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -171,7 +171,7 @@ describe('Download Tests', () => { const fileContents = Buffer.from('try, try again\n', defaultEncoding) const targetPath = path.join(root, `FileC-${statusCode}.txt`) - setupDownloadItemResponse(false, statusCode, fileContents) + setupDownloadItemResponse(false, statusCode, false, fileContents) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -188,6 +188,52 @@ describe('Download Tests', () => { } }) + 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 */ @@ -253,17 +299,24 @@ describe('Download Tests', () => { function setupDownloadItemResponse( isGzip: boolean, firstHttpResponseCode: number, + truncateFirstResponse: boolean, fileContents: Buffer ): void { const spyInstance = jest .spyOn(HttpClient.prototype, 'get') .mockImplementationOnce(async () => { if (firstHttpResponseCode === 200) { + const fullResponse = await constructResponse(isGzip, fileContents) + const actualResponse = truncateFirstResponse + ? fullResponse.subarray(0, 3) + : fullResponse + return { message: getDownloadResponseMessage( firstHttpResponseCode, isGzip, - await constructResponse(isGzip, fileContents) + fullResponse.length, + actualResponse ), readBody: emptyMockReadBody } @@ -272,6 +325,7 @@ describe('Download Tests', () => { message: getDownloadResponseMessage( firstHttpResponseCode, false, + 0, null ), readBody: emptyMockReadBody @@ -283,11 +337,13 @@ describe('Download 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, - await constructResponse(isGzip, fileContents) + fullResponse.length, + fullResponse ), readBody: emptyMockReadBody } @@ -311,6 +367,7 @@ describe('Download Tests', () => { function getDownloadResponseMessage( httpResponseCode: number, isGzip: boolean, + contentLength: number, response: Buffer | null ): http.IncomingMessage { let readCallCount = 0 @@ -335,7 +392,9 @@ describe('Download Tests', () => { }) mockMessage.statusCode = httpResponseCode - mockMessage.headers = {} + mockMessage.headers = { + 'content-length': contentLength.toString() + } if (isGzip) { mockMessage.headers['content-encoding'] = 'gzip' diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 70ea4e44..3b00872e 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -264,6 +264,12 @@ export class DownloadHttpClient { const gunzip = zlib.createGunzip() response.message .pipe(gunzip) + .on('error', error => { + core.error( + `An error has been encountered while attempting to decompress a file` + ) + reject(error) + }) .pipe(destinationStream) .on('close', () => { resolve() From 990647a104d537b67b00ed2e29870a035124a3fd Mon Sep 17 00:00:00 2001 From: Chris Sidi Date: Thu, 26 Nov 2020 01:16:06 -0500 Subject: [PATCH 02/11] More error handling --- .../src/internal/download-http-client.ts | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 3b00872e..64631c5c 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -263,12 +263,20 @@ export class DownloadHttpClient { if (isGzip) { const gunzip = zlib.createGunzip() response.message + .on('error', error => { + 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 has been encountered while attempting to decompress a file` + `An error occurred while attempting to decompress the response stream` ) reject(error) + destinationStream.close() }) .pipe(destinationStream) .on('close', () => { @@ -276,19 +284,26 @@ export class DownloadHttpClient { }) .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) }) From 6b83f0554a8664b6c7b5ce1329741cab5e604abe Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Thu, 3 Dec 2020 10:13:36 -0500 Subject: [PATCH 03/11] Adding retry when we received 200 If we received 200, we will attempt to download the stream. If the stream is gzipped, gzip should throw an error when trying to decompress the stream; or if the stream is truncated, the received bytes should be different from the value set in content-length header. --- packages/artifact/__tests__/download.test.ts | 28 ++++++--- .../src/internal/download-http-client.ts | 57 ++++++++++++++++--- packages/artifact/src/internal/utils.ts | 9 +++ 3 files changed, 79 insertions(+), 15 deletions(-) diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts index 81759d46..518cfc59 100644 --- a/packages/artifact/__tests__/download.test.ts +++ b/packages/artifact/__tests__/download.test.ts @@ -124,7 +124,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileA.txt') - setupDownloadItemResponse(true, 200, false, fileContents) + setupDownloadItemResponse(true, 200, false, fileContents, false) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -147,7 +147,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileB.txt') - setupDownloadItemResponse(false, 200, false, fileContents) + setupDownloadItemResponse(false, 200, false, fileContents, false) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -171,7 +171,7 @@ describe('Download Tests', () => { const fileContents = Buffer.from('try, try again\n', defaultEncoding) const targetPath = path.join(root, `FileC-${statusCode}.txt`) - setupDownloadItemResponse(false, statusCode, false, fileContents) + setupDownloadItemResponse(false, statusCode, false, fileContents, true) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -195,7 +195,8 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileD.txt') - setupDownloadItemResponse(true, 200, true, fileContents) + setupDownloadItemResponse(true, 200, true, fileContents, true) + setupThrowWhenReadingStream() const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -218,7 +219,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileE.txt') - setupDownloadItemResponse(false, 200, true, fileContents) + setupDownloadItemResponse(false, 200, true, fileContents, true) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -291,6 +292,18 @@ describe('Download Tests', () => { }) } + function setupThrowWhenReadingStream(): void { + const spyInstance = jest + .spyOn(DownloadHttpClient.prototype, 'pipeResponseToFile') + .mockImplementationOnce(async () => { + return new Promise(resolve => { + throw new Error( + 'simulate GZip reading truncated buffer and throw Z_BUF_ERROR' + ) + }) + }) + } + /** * Setups up HTTP GET response for downloading items * @param isGzip is the downloaded item gzip encoded @@ -300,7 +313,8 @@ describe('Download Tests', () => { isGzip: boolean, firstHttpResponseCode: number, truncateFirstResponse: boolean, - fileContents: Buffer + fileContents: Buffer, + retryExpected: boolean ): void { const spyInstance = jest .spyOn(HttpClient.prototype, 'get') @@ -334,7 +348,7 @@ describe('Download Tests', () => { }) // set up a second mock only if we expect a retry. Otherwise this mock will affect other tests. - if (firstHttpResponseCode !== 200) { + if (retryExpected) { spyInstance.mockImplementationOnce(async () => { // chained response, if the HTTP GET function gets called again, return a successful response const fullResponse = await constructResponse(isGzip, fileContents) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 64631c5c..2110b6a6 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -9,7 +9,9 @@ import { isThrottledStatusCode, getExponentialRetryTimeInMilliseconds, tryGetRetryAfterValueTimeInMilliseconds, - displayHttpDiagnostics + displayHttpDiagnostics, + getFileSize, + rmFile } from './utils' import {URL} from 'url' import {StatusReporter} from './status-reporter' @@ -151,7 +153,7 @@ export class DownloadHttpClient { ): Promise { let retryCount = 0 const retryLimit = getRetryLimit() - const destinationStream = fs.createWriteStream(downloadPath) + let destinationStream = fs.createWriteStream(downloadPath) const headers = getDownloadHeaders('application/json', true, true) // a single GET request is used to download a file @@ -201,6 +203,27 @@ export class DownloadHttpClient { } } + const isAllBytesReceived = ( + expected?: string, + received?: number + ): boolean => { + // be lenient, if any input is missing, assume success, i.e. not truncated + if ( + !expected || + !received || + process.env['ACTIONS_ARTIFACT_SKIP_DOWNLOAD_VALIDATION'] + ) { + return true + } + + return parseInt(expected) === received + } + + const resetDestinationStream = async (downloadPath: string) => { + await rmFile(downloadPath) + destinationStream = fs.createWriteStream(downloadPath) + } + // keep trying to download a file until a retry limit has been reached while (retryCount <= retryLimit) { let response: IHttpClientResponse @@ -217,19 +240,37 @@ export class DownloadHttpClient { continue } + let forceRetry = false if (isSuccessStatusCode(response.message.statusCode)) { // The body contains the contents of the file however calling response.readBody() causes all the content to be converted to a string // which can cause some gzip encoded data to be lost // Instead of using response.readBody(), response.message is a readableStream that can be directly used to get the raw body contents - return this.pipeResponseToFile( - response, - destinationStream, - isGzip(response.message.headers) - ) - } else if (isRetryableStatusCode(response.message.statusCode)) { + try { + const isGzipped = isGzip(response.message.headers) + await this.pipeResponseToFile(response, destinationStream, isGzipped) + + if ( + isGzipped || + isAllBytesReceived( + response.message.headers['content-length'], + await getFileSize(downloadPath) + ) + ) { + return + } else { + forceRetry = true + } + } catch (error) { + // retry on error, most likely streams were corrupted + forceRetry = true + } + } + + if (forceRetry || isRetryableStatusCode(response.message.statusCode)) { core.info( `A ${response.message.statusCode} response code has been received while attempting to download an artifact` ) + resetDestinationStream(downloadPath) // if a throttled status code is received, try to get the retryAfter header value, else differ to standard exponential backoff isThrottledStatusCode(response.message.statusCode) ? await backOff( diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts index a9f76b24..ea203644 100644 --- a/packages/artifact/src/internal/utils.ts +++ b/packages/artifact/src/internal/utils.ts @@ -303,6 +303,15 @@ export async function createEmptyFilesForArtifact( } } +export async function getFileSize(filePath: string): Promise { + const stats = await fs.stat(filePath) + return stats.size +} + +export async function rmFile(filePath: string): Promise { + await fs.unlink(filePath) +} + export function getProperRetention( retentionInput: number, retentionSetting: string | undefined From b55731c11b1f88efc315881e443661a3129f89b4 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Thu, 3 Dec 2020 10:33:10 -0500 Subject: [PATCH 04/11] Fix styling --- packages/artifact/__tests__/download.test.ts | 4 ++-- packages/artifact/src/internal/download-http-client.ts | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts index 518cfc59..b940fdec 100644 --- a/packages/artifact/__tests__/download.test.ts +++ b/packages/artifact/__tests__/download.test.ts @@ -293,10 +293,10 @@ describe('Download Tests', () => { } function setupThrowWhenReadingStream(): void { - const spyInstance = jest + jest .spyOn(DownloadHttpClient.prototype, 'pipeResponseToFile') .mockImplementationOnce(async () => { - return new Promise(resolve => { + return new Promise(() => { throw new Error( 'simulate GZip reading truncated buffer and throw Z_BUF_ERROR' ) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 2110b6a6..7caef993 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -219,7 +219,9 @@ export class DownloadHttpClient { return parseInt(expected) === received } - const resetDestinationStream = async (downloadPath: string) => { + const resetDestinationStream = async ( + downloadPath: string + ): Promise => { await rmFile(downloadPath) destinationStream = fs.createWriteStream(downloadPath) } From 8ed9455d68df6963c7c9f088c4d26ceced86aa50 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Thu, 3 Dec 2020 10:57:04 -0500 Subject: [PATCH 05/11] More styling fix --- packages/artifact/src/internal/download-http-client.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 7caef993..201a5f70 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -220,10 +220,10 @@ export class DownloadHttpClient { } const resetDestinationStream = async ( - downloadPath: string + fileDownloadPath: string ): Promise => { - await rmFile(downloadPath) - destinationStream = fs.createWriteStream(downloadPath) + await rmFile(fileDownloadPath) + destinationStream = fs.createWriteStream(fileDownloadPath) } // keep trying to download a file until a retry limit has been reached From 05b1692026b7ef47798e13e73cc9dacd06ad3305 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Thu, 3 Dec 2020 12:40:37 -0500 Subject: [PATCH 06/11] Add some debugging statements --- packages/artifact/src/internal/download-http-client.ts | 8 ++++++++ packages/artifact/src/internal/utils.ts | 3 +++ 2 files changed, 11 insertions(+) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 201a5f70..10b59ddf 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -213,6 +213,11 @@ export class DownloadHttpClient { !received || process.env['ACTIONS_ARTIFACT_SKIP_DOWNLOAD_VALIDATION'] ) { + if (process.env['ACTIONS_ARTIFACT_SKIP_DOWNLOAD_VALIDATION']) { + core.info( + 'Skipping download validation since environment variable is set' + ) + } return true } @@ -231,6 +236,9 @@ export class DownloadHttpClient { let response: IHttpClientResponse try { response = await makeDownloadRequest() + if (core.isDebug()) { + displayHttpDiagnostics(response) + } } catch (error) { // if an error is caught, it is usually indicative of a timeout so retry the download core.info('An error occurred while attempting to download a file') diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts index ea203644..2f4f2f47 100644 --- a/packages/artifact/src/internal/utils.ts +++ b/packages/artifact/src/internal/utils.ts @@ -305,6 +305,9 @@ export async function createEmptyFilesForArtifact( export async function getFileSize(filePath: string): Promise { const stats = await fs.stat(filePath) + debug( + `${filePath} size:(${stats.size}) blksize:(${stats.blksize}) blocks:(${stats.blocks})` + ) return stats.size } From d4e990d92ff6327891726a6446253aed9d2d1a01 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Thu, 3 Dec 2020 13:09:43 -0500 Subject: [PATCH 07/11] Always print skip validation log --- packages/artifact/src/internal/download-http-client.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 10b59ddf..7fb06a4b 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -213,11 +213,7 @@ export class DownloadHttpClient { !received || process.env['ACTIONS_ARTIFACT_SKIP_DOWNLOAD_VALIDATION'] ) { - if (process.env['ACTIONS_ARTIFACT_SKIP_DOWNLOAD_VALIDATION']) { - core.info( - 'Skipping download validation since environment variable is set' - ) - } + core.info('Skipping download validation.') return true } From a600dd34a5375a628d0123c69ae54366d8037079 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Thu, 3 Dec 2020 13:34:59 -0500 Subject: [PATCH 08/11] Try close stream before unlink --- packages/artifact/src/internal/download-http-client.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index 7fb06a4b..bd409ebb 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -223,6 +223,7 @@ export class DownloadHttpClient { const resetDestinationStream = async ( fileDownloadPath: string ): Promise => { + destinationStream.close() await rmFile(fileDownloadPath) destinationStream = fs.createWriteStream(fileDownloadPath) } From dc491a61cae3e56f9b7abdc98b1aed54b3c3cb69 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Fri, 4 Dec 2020 09:19:58 -0500 Subject: [PATCH 09/11] pipeResponseToFile already throws when reading truncated stream --- packages/artifact/__tests__/download.test.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts index b940fdec..ec07a1fe 100644 --- a/packages/artifact/__tests__/download.test.ts +++ b/packages/artifact/__tests__/download.test.ts @@ -196,7 +196,6 @@ describe('Download Tests', () => { const targetPath = path.join(root, 'FileD.txt') setupDownloadItemResponse(true, 200, true, fileContents, true) - setupThrowWhenReadingStream() const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -292,18 +291,6 @@ describe('Download Tests', () => { }) } - function setupThrowWhenReadingStream(): void { - jest - .spyOn(DownloadHttpClient.prototype, 'pipeResponseToFile') - .mockImplementationOnce(async () => { - return new Promise(() => { - throw new Error( - 'simulate GZip reading truncated buffer and throw Z_BUF_ERROR' - ) - }) - }) - } - /** * Setups up HTTP GET response for downloading items * @param isGzip is the downloaded item gzip encoded From 5be846b72d43a6600814493b6da77a841c28a3c9 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Fri, 4 Dec 2020 09:41:00 -0500 Subject: [PATCH 10/11] style change, refactor arg sequence --- packages/artifact/__tests__/download.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/artifact/__tests__/download.test.ts b/packages/artifact/__tests__/download.test.ts index ec07a1fe..b77e5a2b 100644 --- a/packages/artifact/__tests__/download.test.ts +++ b/packages/artifact/__tests__/download.test.ts @@ -124,7 +124,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileA.txt') - setupDownloadItemResponse(true, 200, false, fileContents, false) + setupDownloadItemResponse(fileContents, true, 200, false, false) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -147,7 +147,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileB.txt') - setupDownloadItemResponse(false, 200, false, fileContents, false) + setupDownloadItemResponse(fileContents, false, 200, false, false) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -171,7 +171,7 @@ describe('Download Tests', () => { const fileContents = Buffer.from('try, try again\n', defaultEncoding) const targetPath = path.join(root, `FileC-${statusCode}.txt`) - setupDownloadItemResponse(false, statusCode, false, fileContents, true) + setupDownloadItemResponse(fileContents, false, statusCode, false, true) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -195,7 +195,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileD.txt') - setupDownloadItemResponse(true, 200, true, fileContents, true) + setupDownloadItemResponse(fileContents, true, 200, true, true) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -218,7 +218,7 @@ describe('Download Tests', () => { ) const targetPath = path.join(root, 'FileE.txt') - setupDownloadItemResponse(false, 200, true, fileContents, true) + setupDownloadItemResponse(fileContents, false, 200, true, true) const downloadHttpClient = new DownloadHttpClient() const items: DownloadItem[] = [] @@ -297,10 +297,10 @@ describe('Download Tests', () => { * @param firstHttpResponseCode the http response code that should be returned */ function setupDownloadItemResponse( + fileContents: Buffer, isGzip: boolean, firstHttpResponseCode: number, truncateFirstResponse: boolean, - fileContents: Buffer, retryExpected: boolean ): void { const spyInstance = jest From b593d1deb4823fe44c75a84ad7ab3547753ec920 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Fri, 4 Dec 2020 11:03:17 -0500 Subject: [PATCH 11/11] Close destination steam before reject Co-authored-by: Chris Sidi --- packages/artifact/src/internal/download-http-client.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts index bd409ebb..e090c547 100644 --- a/packages/artifact/src/internal/download-http-client.ts +++ b/packages/artifact/src/internal/download-http-client.ts @@ -315,16 +315,17 @@ export class DownloadHttpClient { core.error( `An error occurred while attempting to read the response stream` ) - reject(error) gunzip.close() + destinationStream.close() + reject(error) }) .pipe(gunzip) .on('error', error => { core.error( `An error occurred while attempting to decompress the response stream` ) - reject(error) destinationStream.close() + reject(error) }) .pipe(destinationStream) .on('close', () => { @@ -342,8 +343,8 @@ export class DownloadHttpClient { core.error( `An error occurred while attempting to read the response stream` ) - reject(error) destinationStream.close() + reject(error) }) .pipe(destinationStream) .on('close', () => {