diff --git a/packages/core/src/shared/errors.ts b/packages/core/src/shared/errors.ts index 54841b7622f..424c5c5db3d 100644 --- a/packages/core/src/shared/errors.ts +++ b/packages/core/src/shared/errors.ts @@ -16,6 +16,7 @@ import { CodeWhispererStreamingServiceException } from '@amzn/codewhisperer-stre import { driveLetterRegex } from './utilities/pathUtils' import { getLogger } from './logger/logger' import { crashMonitoringDirName } from './constants' +import { RequestCancelledError } from './request' let _username = 'unknown-user' let _isAutomation = false @@ -618,7 +619,11 @@ function hasTime(error: Error): error is typeof error & { time: Date } { } export function isUserCancelledError(error: unknown): boolean { - return CancellationError.isUserCancelled(error) || (error instanceof ToolkitError && error.cancelled) + return ( + CancellationError.isUserCancelled(error) || + (error instanceof ToolkitError && error.cancelled) || + error instanceof RequestCancelledError + ) } /** diff --git a/packages/core/src/shared/lsp/lspResolver.ts b/packages/core/src/shared/lsp/lspResolver.ts index d85ee73b929..80ad2fe0b72 100644 --- a/packages/core/src/shared/lsp/lspResolver.ts +++ b/packages/core/src/shared/lsp/lspResolver.ts @@ -17,6 +17,9 @@ import { HttpResourceFetcher } from '../resourcefetcher/httpResourceFetcher' import { showMessageWithCancel } from '../../shared/utilities/messages' import { Timeout } from '../utilities/timeoutUtils' +// max timeout for downloading remote LSP assets progress, the lowest possible is 3000, bounded by httpResourceFetcher's waitUntil +const remoteDownloadTimeout = 5000 + export class LanguageServerResolver { constructor( private readonly manifest: Manifest, @@ -33,8 +36,6 @@ export class LanguageServerResolver { * @throws ToolkitError if no compatible version can be found */ async resolve() { - const timeout = new Timeout(5000) - await showMessageWithCancel(`Downloading '${this.lsName}' language server`, timeout) function getServerVersion(result: LspResult) { return { languageServerVersion: result.version, @@ -63,7 +64,6 @@ export class LanguageServerResolver { return await tryStageResolvers('getServer', serverResolvers, getServerVersion) } finally { logger.info(`Finished setting up LSP server`) - timeout.cancel() } } @@ -87,19 +87,34 @@ export class LanguageServerResolver { } } + /** + * Show a toast notification with progress bar for lsp remote downlaod + * Returns a timeout to be passed down into httpFetcher to handle user cancellation + */ + private async showDownloadProgress() { + const timeout = new Timeout(remoteDownloadTimeout) + await showMessageWithCancel(`Downloading '${this.lsName}' language server`, timeout) + return timeout + } + private async fetchRemoteServer( cacheDirectory: string, latestVersion: LspVersion, targetContents: TargetContent[] ): Promise { - if (await this.downloadRemoteTargetContent(targetContents, latestVersion.serverVersion)) { - return { - location: 'remote', - version: latestVersion.serverVersion, - assetDirectory: cacheDirectory, + const timeout = await this.showDownloadProgress() + try { + if (await this.downloadRemoteTargetContent(targetContents, latestVersion.serverVersion, timeout)) { + return { + location: 'remote', + version: latestVersion.serverVersion, + assetDirectory: cacheDirectory, + } + } else { + throw new ToolkitError('Failed to download server from remote', { code: 'RemoteDownloadFailed' }) } - } else { - throw new ToolkitError('Failed to download server from remote', { code: 'RemoteDownloadFailed' }) + } finally { + timeout.dispose() } } @@ -193,7 +208,7 @@ export class LanguageServerResolver { * true, if all of the contents were successfully downloaded and unzipped * false, if any of the contents failed to download or unzip */ - private async downloadRemoteTargetContent(contents: TargetContent[], version: string) { + private async downloadRemoteTargetContent(contents: TargetContent[], version: string, timeout: Timeout) { const downloadDirectory = this.getDownloadDirectory(version) if (!(await fs.existsDir(downloadDirectory))) { @@ -202,7 +217,11 @@ export class LanguageServerResolver { const fetchTasks = contents.map(async (content) => { return { - res: await new HttpResourceFetcher(content.url, { showUrl: true }).get(), + res: await new HttpResourceFetcher(content.url, { + showUrl: true, + timeout: timeout, + throwOnError: true, + }).get(), hash: content.hashes[0], filename: content.filename, } diff --git a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts index 1a2300cdcb9..18cc696770f 100644 --- a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts +++ b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts @@ -8,6 +8,7 @@ import { getLogger, Logger } from '../logger/logger' import { ResourceFetcher } from './resourcefetcher' import { Timeout, CancelEvent, waitUntil } from '../utilities/timeoutUtils' import request, { RequestError } from '../request' +import { isUserCancelledError } from '../errors' type RequestHeaders = { eTag?: string; gZip?: boolean } @@ -22,6 +23,7 @@ export class HttpResourceFetcher implements ResourceFetcher { * @param {string} params.friendlyName If URL is not shown, replaces the URL with this text. * @param {Timeout} params.timeout Timeout token to abort/cancel the request. Similar to `AbortSignal`. * @param {number} params.retries The number of retries a get request should make if one fails + * @param {boolean} params.throwOnError True if we want to throw if there's request error, defaul to false */ public constructor( private readonly url: string, @@ -29,8 +31,11 @@ export class HttpResourceFetcher implements ResourceFetcher { showUrl: boolean friendlyName?: string timeout?: Timeout + throwOnError?: boolean } - ) {} + ) { + this.params.throwOnError = this.params.throwOnError ?? false + } /** * Returns the response of the resource, or undefined if the response failed could not be retrieved. @@ -82,6 +87,9 @@ export class HttpResourceFetcher implements ResourceFetcher { `Error downloading ${this.logText()}: %s`, error.message ?? error.code ?? error.response.statusText ?? error.response.status ) + if (this.params.throwOnError) { + throw error + } return undefined } } @@ -112,7 +120,10 @@ export class HttpResourceFetcher implements ResourceFetcher { timeout: 3000, interval: 100, backoff: 2, - retryOnFail: true, + retryOnFail: (error: Error) => { + // Retry unless the user intentionally canceled the operation. + return !isUserCancelledError(error) + }, } ) } diff --git a/packages/core/src/shared/utilities/timeoutUtils.ts b/packages/core/src/shared/utilities/timeoutUtils.ts index 7c1a6e521ca..8a8e0226879 100644 --- a/packages/core/src/shared/utilities/timeoutUtils.ts +++ b/packages/core/src/shared/utilities/timeoutUtils.ts @@ -223,11 +223,12 @@ interface WaitUntilOptions { readonly backoff?: number /** * Only retries when an error is thrown, otherwise returning the immediate result. + * Can also be a callback for conditional retry based on errors * - 'truthy' arg is ignored * - If the timeout is reached it throws the last error * - default: false */ - readonly retryOnFail?: boolean + readonly retryOnFail?: boolean | ((error: Error) => boolean) } export const waitUntilDefaultTimeout = 2000 @@ -247,6 +248,11 @@ export async function waitUntil( fn: () => Promise, options: WaitUntilOptions & { retryOnFail: false } ): Promise +export async function waitUntil( + fn: () => Promise, + options: WaitUntilOptions & { retryOnFail: (error: Error) => boolean } +): Promise + export async function waitUntil( fn: () => Promise, options: Omit @@ -267,6 +273,17 @@ export async function waitUntil(fn: () => Promise, options: WaitUntilOptio let elapsed: number = 0 let remaining = opt.timeout + // Internal helper to determine if we should retry + function shouldRetry(error: Error | undefined): boolean { + if (error === undefined) { + return typeof opt.retryOnFail === 'boolean' ? opt.retryOnFail : true + } + if (typeof opt.retryOnFail === 'function') { + return opt.retryOnFail(error) + } + return opt.retryOnFail + } + for (let i = 0; true; i++) { const start: number = globals.clock.Date.now() let result: T @@ -279,16 +296,16 @@ export async function waitUntil(fn: () => Promise, options: WaitUntilOptio result = await fn() } - if (opt.retryOnFail || (opt.truthy && result) || (!opt.truthy && result !== undefined)) { + if (shouldRetry(lastError) || (opt.truthy && result) || (!opt.truthy && result !== undefined)) { return result } } catch (e) { - if (!opt.retryOnFail) { + // Unlikely to hit this, but exists for typing + if (!(e instanceof Error)) { throw e } - // Unlikely to hit this, but exists for typing - if (!(e instanceof Error)) { + if (!shouldRetry(e)) { throw e } @@ -300,10 +317,9 @@ export async function waitUntil(fn: () => Promise, options: WaitUntilOptio // If the sleep will exceed the timeout, abort early if (elapsed + interval >= remaining) { - if (!opt.retryOnFail) { + if (!shouldRetry(lastError)) { return undefined } - throw lastError } diff --git a/packages/core/src/test/shared/utilities/timeoutUtils.test.ts b/packages/core/src/test/shared/utilities/timeoutUtils.test.ts index 3ee5968132b..71d854d3cc2 100644 --- a/packages/core/src/test/shared/utilities/timeoutUtils.test.ts +++ b/packages/core/src/test/shared/utilities/timeoutUtils.test.ts @@ -394,6 +394,46 @@ export const timeoutUtilsDescribe = describe('timeoutUtils', async function () { fn = sandbox.stub() }) + it('should retry when retryOnFail callback returns true', async function () { + fn.onCall(0).throws(new Error('Retry error')) + fn.onCall(1).throws(new Error('Retry error')) + fn.onCall(2).resolves('success') + + const res = waitUntil(fn, { + retryOnFail: (error: Error) => { + return error.message === 'Retry error' + }, + }) + + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) + assert.strictEqual(fn.callCount, 2) + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) + assert.strictEqual(fn.callCount, 3) + assert.strictEqual(await res, 'success') + }) + + it('should not retry when retryOnFail callback returns false', async function () { + fn.onCall(0).throws(new Error('Retry error')) + fn.onCall(1).throws(new Error('Retry error')) + fn.onCall(2).throws(new Error('Last error')) + fn.onCall(3).resolves('this is not hit') + + const res = assert.rejects( + waitUntil(fn, { + retryOnFail: (error: Error) => { + return error.message === 'Retry error' + }, + }), + (e) => e instanceof Error && e.message === 'Last error' + ) + + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) + assert.strictEqual(fn.callCount, 2) + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) + assert.strictEqual(fn.callCount, 3) + await res + }) + it('retries the function until it succeeds', async function () { fn.onCall(0).throws() fn.onCall(1).throws()