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.

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

Introducing timeout in segment download to avoid the download stuck problem #1140

Merged
merged 19 commits into from
Aug 9, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
13,572 changes: 6,701 additions & 6,871 deletions package-lock.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"eslint-plugin-jest": "^22.21.0",
"flow-bin": "^0.115.0",
"jest": "^27.2.5",
"lerna": "^4.0.0",
"lerna": "^5.3.0",
"prettier": "^1.19.1",
"ts-jest": "^27.0.5",
"typescript": "^3.9.9"
Expand Down
5 changes: 4 additions & 1 deletion packages/cache/RELEASES.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,7 @@

### 3.0.1
- Fix [#833](https://github.com/actions/cache/issues/833) - cache doesn't work with github workspace directory.
- Fix [#809](https://github.com/actions/cache/issues/809) `zstd -d: no such file or directory` error on AWS self-hosted runners.
- Fix [#809](https://github.com/actions/cache/issues/809) `zstd -d: no such file or directory` error on AWS self-hosted runners.

### 3.0.2
- Added 1 hour timeout for the download stuck issue [#810](https://github.com/actions/cache/issues/810).
7 changes: 5 additions & 2 deletions packages/cache/__tests__/options.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
const useAzureSdk = true
const downloadConcurrency = 8
const timeoutInMs = 30000
const abortTimeInMs = 3600000
const uploadConcurrency = 4
const uploadChunkSize = 32 * 1024 * 1024

Expand All @@ -17,15 +18,17 @@ test('getDownloadOptions sets defaults', async () => {
expect(actualOptions).toEqual({
useAzureSdk,
downloadConcurrency,
timeoutInMs
timeoutInMs,
abortTimeInMs
})
})

test('getDownloadOptions overrides all settings', async () => {
const expectedOptions: DownloadOptions = {
useAzureSdk: false,
downloadConcurrency: 14,
timeoutInMs: 20000
timeoutInMs: 20000,
abortTimeInMs: 3600000
}

const actualOptions = getDownloadOptions(expectedOptions)
Expand Down
4 changes: 2 additions & 2 deletions packages/cache/package-lock.json

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

2 changes: 1 addition & 1 deletion packages/cache/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@actions/cache",
"version": "3.0.1",
"version": "3.0.2",
"preview": true,
"description": "Actions cache lib",
"keywords": [
Expand Down
40 changes: 31 additions & 9 deletions packages/cache/src/internal/downloadUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import {SocketTimeout} from './constants'
import {DownloadOptions} from '../options'
import {retryHttpClientResponse} from './requestUtils'

import {AbortController} from '@azure/abort-controller'

/**
* Pipes the body of a HTTP response to a stream
*
Expand Down Expand Up @@ -247,7 +249,7 @@ export async function downloadCacheStorageSDK(

try {
downloadProgress.startDisplayTimer()

const controller = new AbortController()
while (!downloadProgress.isDone()) {
const segmentStart =
downloadProgress.segmentOffset + downloadProgress.segmentSize
Expand All @@ -258,21 +260,41 @@ export async function downloadCacheStorageSDK(
)

downloadProgress.nextSegment(segmentSize)

const result = await client.downloadToBuffer(
segmentStart,
segmentSize,
{
const abortTimeInMs =
options.abortTimeInMs === undefined ? 3600000 : options.abortTimeInMs
const result = await promiseWithTimeout(
kotewar marked this conversation as resolved.
Show resolved Hide resolved
abortTimeInMs,
client.downloadToBuffer(segmentStart, segmentSize, {
abortSignal: controller.signal,
kotewar marked this conversation as resolved.
Show resolved Hide resolved
concurrency: options.downloadConcurrency,
onProgress: downloadProgress.onProgress()
}
})
)

fs.writeFileSync(fd, result)
if (result === 'timeout') {
controller.abort()
throw new Error('Download aborted, segment download timed out.')
} else if (Buffer.isBuffer(result)) {
fs.writeFileSync(fd, result)
}
}
} finally {
downloadProgress.stopDisplayTimer()
fs.closeSync(fd)
}
}
}

const promiseWithTimeout = async (
timeoutMs: number,
promise: Promise<Buffer>
): Promise<unknown> => {
let timeoutHandle: NodeJS.Timeout
const timeoutPromise = new Promise(resolve => {
timeoutHandle = setTimeout(() => resolve('timeout'), timeoutMs)
})

return Promise.race([promise, timeoutPromise]).then(result => {
clearTimeout(timeoutHandle)
kotewar marked this conversation as resolved.
Show resolved Hide resolved
return result
})
}
15 changes: 14 additions & 1 deletion packages/cache/src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ export interface DownloadOptions {
* @default 30000
*/
timeoutInMs?: number

/**
* Time after which download should be aborted if stuck
*
* @default 3600000
*/
abortTimeInMs?: number
kotewar marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -84,7 +91,8 @@ export function getDownloadOptions(copy?: DownloadOptions): DownloadOptions {
const result: DownloadOptions = {
useAzureSdk: true,
downloadConcurrency: 8,
timeoutInMs: 30000
timeoutInMs: 30000,
abortTimeInMs: 3600000
}

if (copy) {
Expand All @@ -99,11 +107,16 @@ export function getDownloadOptions(copy?: DownloadOptions): DownloadOptions {
if (typeof copy.timeoutInMs === 'number') {
result.timeoutInMs = copy.timeoutInMs
}

if (typeof copy.abortTimeInMs === 'number') {
result.abortTimeInMs = copy.abortTimeInMs
}
}

core.debug(`Use Azure SDK: ${result.useAzureSdk}`)
core.debug(`Download concurrency: ${result.downloadConcurrency}`)
core.debug(`Request timeout (ms): ${result.timeoutInMs}`)
core.debug(`Abort time (ms): ${result.abortTimeInMs}`)

return result
}