From 7cac92ef6b2aa96d3ee5e1fd7bb71bc608a9703d Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Thu, 14 May 2020 14:41:27 +0200 Subject: [PATCH 1/3] Correctly reset chunk during artifact upload on retry --- .github/workflows/artifact-tests.yml | 11 ++++-- .../src/internal/upload-http-client.ts | 34 +++++++++++-------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/.github/workflows/artifact-tests.yml b/.github/workflows/artifact-tests.yml index 8b45404a59..66e51ff9c0 100644 --- a/.github/workflows/artifact-tests.yml +++ b/.github/workflows/artifact-tests.yml @@ -1,5 +1,12 @@ name: artifact-unit-tests -on: push +push: + branches: + - master + paths-ignore: + - '**.md' +pull_request: + paths-ignore: + - '**.md' jobs: build: @@ -7,7 +14,7 @@ jobs: strategy: matrix: - runs-on: [ubuntu-latest, windows-latest, macOS-latest] + runs-on: [ubuntu-latest, windows-latest, macos-latest] fail-fast: false runs-on: ${{ matrix.runs-on }} diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index dc2d331899..3733e1aed5 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -208,25 +208,30 @@ export class UploadHttpClient { // for creating a new GZip file, an in-memory buffer is used for compression if (totalFileSize < 65536) { const buffer = await createGZipFileInBuffer(parameters.file) - let uploadStream: NodeJS.ReadableStream + + //An open stream is needed in the event of a failure and we need to retry. If a NodeJS.ReadableStream is directly passed in, + // it will not properly get reset to the start of the stream if a chunk upload needs to be retried + let openUploadStream: () => NodeJS.ReadableStream if (totalFileSize < buffer.byteLength) { // compression did not help with reducing the size, use a readable stream from the original file for upload - uploadStream = fs.createReadStream(parameters.file) + openUploadStream = () => fs.createReadStream(parameters.file) isGzip = false uploadFileSize = totalFileSize } else { // create a readable stream using a PassThrough stream that is both readable and writable - const passThrough = new stream.PassThrough() - passThrough.end(buffer) - uploadStream = passThrough + openUploadStream = () => { + const passThrough = new stream.PassThrough() + passThrough.end(buffer) + return passThrough + } uploadFileSize = buffer.byteLength } const result = await this.uploadChunk( httpClientIndex, parameters.resourceUrl, - uploadStream, + openUploadStream, 0, uploadFileSize - 1, uploadFileSize, @@ -296,11 +301,12 @@ export class UploadHttpClient { const result = await this.uploadChunk( httpClientIndex, parameters.resourceUrl, - fs.createReadStream(uploadFilePath, { - start, - end, - autoClose: false - }), + () => + fs.createReadStream(uploadFilePath, { + start, + end, + autoClose: false + }), start, end, uploadFileSize, @@ -335,7 +341,7 @@ export class UploadHttpClient { * indicates a retryable status, we try to upload the chunk as well * @param {number} httpClientIndex The index of the httpClient being used to make all the necessary calls * @param {string} resourceUrl Url of the resource that the chunk will be uploaded to - * @param {NodeJS.ReadableStream} data Stream of the file that will be uploaded + * @param {NodeJS.ReadableStream} openStream Stream of the file that will be uploaded * @param {number} start Starting byte index of file that the chunk belongs to * @param {number} end Ending byte index of file that the chunk belongs to * @param {number} uploadFileSize Total size of the file in bytes that is being uploaded @@ -346,7 +352,7 @@ export class UploadHttpClient { private async uploadChunk( httpClientIndex: number, resourceUrl: string, - data: NodeJS.ReadableStream, + openStream: () => NodeJS.ReadableStream, start: number, end: number, uploadFileSize: number, @@ -365,7 +371,7 @@ export class UploadHttpClient { const uploadChunkRequest = async (): Promise => { const client = this.uploadHttpManager.getClient(httpClientIndex) - return await client.sendStream('PUT', resourceUrl, data, headers) + return await client.sendStream('PUT', resourceUrl, openStream(), headers) } let retryCount = 0 From 5bacfcced8945f5d8908918baa0bd34f24f4f045 Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Thu, 14 May 2020 15:39:51 +0200 Subject: [PATCH 2/3] Update workflow --- .github/workflows/artifact-tests.yml | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/.github/workflows/artifact-tests.yml b/.github/workflows/artifact-tests.yml index 66e51ff9c0..20890ed000 100644 --- a/.github/workflows/artifact-tests.yml +++ b/.github/workflows/artifact-tests.yml @@ -1,12 +1,13 @@ name: artifact-unit-tests -push: - branches: - - master - paths-ignore: - - '**.md' -pull_request: - paths-ignore: - - '**.md' +on: + push: + branches: + - master + paths-ignore: + - '**.md' + pull_request: + paths-ignore: + - '**.md' jobs: build: From 296a75104cc117a87b80bab8a03b7f22450b6ba4 Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Thu, 14 May 2020 22:11:06 +0200 Subject: [PATCH 3/3] Implementation details around the passthrough stream --- packages/artifact/docs/implementation-details.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/artifact/docs/implementation-details.md b/packages/artifact/docs/implementation-details.md index bed96f0bf4..9ab1b0065e 100644 --- a/packages/artifact/docs/implementation-details.md +++ b/packages/artifact/docs/implementation-details.md @@ -6,6 +6,10 @@ Warning: Implementation details may change at any time without notice. This is m ![image](https://user-images.githubusercontent.com/16109154/79765587-19522b00-8327-11ea-9679-410bb10e1b13.png) +During artifact upload, gzip is used to compress individual files that then get uploaded. This is used to minimize the amount of data that gets uploaded which reduces the total amount of HTTP calls (upload happens in 4MB chunks). This results in considerably faster uploads with huge performance implications especially on self-hosted runners. + +If a file is less than 64KB in size, a passthrough stream (readable and writable) is used to convert an in-memory buffer into a readable stream without any extra streams or pipping. + ## Retry Logic when downloading an individual file ![image](https://user-images.githubusercontent.com/16109154/78555461-5be71400-780d-11ea-9abd-b05b77a95a3f.png)