From 07ec9b253445ff8a539da31076bc4210b77ad49a Mon Sep 17 00:00:00 2001 From: tomzu Date: Thu, 13 Feb 2025 11:48:45 -0500 Subject: [PATCH 1/7] feat(amazonq): user cancellable lsp download --- packages/core/src/shared/lsp/lspResolver.ts | 39 ++++++++++++++------- 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/packages/core/src/shared/lsp/lspResolver.ts b/packages/core/src/shared/lsp/lspResolver.ts index d85ee73b929..c5c9cd553fa 100644 --- a/packages/core/src/shared/lsp/lspResolver.ts +++ b/packages/core/src/shared/lsp/lspResolver.ts @@ -33,8 +33,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 +61,6 @@ export class LanguageServerResolver { return await tryStageResolvers('getServer', serverResolvers, getServerVersion) } finally { logger.info(`Finished setting up LSP server`) - timeout.cancel() } } @@ -87,19 +84,37 @@ 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(5000) + timeout?.token.onCancellationRequested((event) => { + logger.info('Remote download cancelled by user') + }) + 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,7 @@ 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 }).get(), hash: content.hashes[0], filename: content.filename, } From 04c7a3592ad61bba082a72798b3092b9ac9f48b9 Mon Sep 17 00:00:00 2001 From: tomzu Date: Thu, 13 Feb 2025 12:40:12 -0500 Subject: [PATCH 2/7] minor refactors --- packages/core/src/shared/lsp/lspResolver.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/src/shared/lsp/lspResolver.ts b/packages/core/src/shared/lsp/lspResolver.ts index c5c9cd553fa..f8aef531dca 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, @@ -89,10 +92,7 @@ export class LanguageServerResolver { * Returns a timeout to be passed down into httpFetcher to handle user cancellation */ private async showDownloadProgress() { - const timeout = new Timeout(5000) - timeout?.token.onCancellationRequested((event) => { - logger.info('Remote download cancelled by user') - }) + const timeout = new Timeout(remoteDownloadTimeout) await showMessageWithCancel(`Downloading '${this.lsName}' language server`, timeout) return timeout } From b4b6a05b7d59f2896efd77652ff7fb9ed4637115 Mon Sep 17 00:00:00 2001 From: tomzu Date: Wed, 26 Feb 2025 14:15:03 -0500 Subject: [PATCH 3/7] let error propogate up and add call back feature to timeout --- packages/core/src/shared/lsp/lspResolver.ts | 14 +++++++-- .../resourcefetcher/httpResourceFetcher.ts | 18 ++++++++--- .../core/src/shared/utilities/timeoutUtils.ts | 30 ++++++++++++++----- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/packages/core/src/shared/lsp/lspResolver.ts b/packages/core/src/shared/lsp/lspResolver.ts index f8aef531dca..25fc767daaa 100644 --- a/packages/core/src/shared/lsp/lspResolver.ts +++ b/packages/core/src/shared/lsp/lspResolver.ts @@ -15,7 +15,8 @@ import { createHash } from '../crypto' import { lspSetupStage, StageResolver, tryStageResolvers } from './utils/setupStage' import { HttpResourceFetcher } from '../resourcefetcher/httpResourceFetcher' import { showMessageWithCancel } from '../../shared/utilities/messages' -import { Timeout } from '../utilities/timeoutUtils' +import { CancellationError, Timeout } from '../utilities/timeoutUtils' +import { RequestCancelledError } from '../request' // max timeout for downloading remote LSP assets progress, the lowest possible is 3000, bounded by httpResourceFetcher's waitUntil const remoteDownloadTimeout = 5000 @@ -113,6 +114,11 @@ export class LanguageServerResolver { } else { throw new ToolkitError('Failed to download server from remote', { code: 'RemoteDownloadFailed' }) } + } catch (err) { + if (err instanceof RequestCancelledError) { + throw new CancellationError('user') + } + throw err } finally { timeout.dispose() } @@ -217,7 +223,11 @@ export class LanguageServerResolver { const fetchTasks = contents.map(async (content) => { return { - res: await new HttpResourceFetcher(content.url, { showUrl: true, timeout: timeout }).get(), + res: await new HttpResourceFetcher(content.url, { + showUrl: true, + timeout: timeout, + swallowError: false, + }).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..1801f4a7b7c 100644 --- a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts +++ b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts @@ -7,7 +7,7 @@ import { VSCODE_EXTENSION_ID } from '../extensions' import { getLogger, Logger } from '../logger/logger' import { ResourceFetcher } from './resourcefetcher' import { Timeout, CancelEvent, waitUntil } from '../utilities/timeoutUtils' -import request, { RequestError } from '../request' +import request, { RequestCancelledError, RequestError } from '../request' type RequestHeaders = { eTag?: string; gZip?: boolean } @@ -22,6 +22,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.swallowError True if we want to swallow the request error, false if we want the error to keep bubbling up */ public constructor( private readonly url: string, @@ -29,8 +30,11 @@ export class HttpResourceFetcher implements ResourceFetcher { showUrl: boolean friendlyName?: string timeout?: Timeout + swallowError?: boolean } - ) {} + ) { + this.params.swallowError = this.params.swallowError ?? true + } /** * Returns the response of the resource, or undefined if the response failed could not be retrieved. @@ -82,7 +86,10 @@ export class HttpResourceFetcher implements ResourceFetcher { `Error downloading ${this.logText()}: %s`, error.message ?? error.code ?? error.response.statusText ?? error.response.status ) - return undefined + if (this.params.swallowError) { + return undefined + } + throw error } } @@ -112,7 +119,10 @@ export class HttpResourceFetcher implements ResourceFetcher { timeout: 3000, interval: 100, backoff: 2, - retryOnFail: true, + retryOnFail: (error: Error) => { + // retry unless we got an user cancellation error + return !(error instanceof RequestCancelledError) + }, } ) } diff --git a/packages/core/src/shared/utilities/timeoutUtils.ts b/packages/core/src/shared/utilities/timeoutUtils.ts index 7c1a6e521ca..552d869a0f7 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 furthur error processing * - '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 } From ecd939eb938d534d8f4d901523feaf02ebb99bc1 Mon Sep 17 00:00:00 2001 From: tomzu Date: Wed, 26 Feb 2025 14:18:53 -0500 Subject: [PATCH 4/7] fix typo --- packages/core/src/shared/utilities/timeoutUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/shared/utilities/timeoutUtils.ts b/packages/core/src/shared/utilities/timeoutUtils.ts index 552d869a0f7..8a8e0226879 100644 --- a/packages/core/src/shared/utilities/timeoutUtils.ts +++ b/packages/core/src/shared/utilities/timeoutUtils.ts @@ -223,7 +223,7 @@ interface WaitUntilOptions { readonly backoff?: number /** * Only retries when an error is thrown, otherwise returning the immediate result. - * Can also be a callback for furthur error processing + * 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 From d6f8e77d5a179c246e3eb5a6526ebdcd3fae233a Mon Sep 17 00:00:00 2001 From: tomzu Date: Thu, 27 Feb 2025 12:32:41 -0500 Subject: [PATCH 5/7] added test for waitUntil --- .../shared/utilities/timeoutUtils.test.ts | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) 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() From fbc0de08799b3eca735e2c38214c0ba2bdafef34 Mon Sep 17 00:00:00 2001 From: tomzu Date: Thu, 27 Feb 2025 13:00:03 -0500 Subject: [PATCH 6/7] renamed to throwOnError --- packages/core/src/shared/lsp/lspResolver.ts | 2 +- .../shared/resourcefetcher/httpResourceFetcher.ts | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/core/src/shared/lsp/lspResolver.ts b/packages/core/src/shared/lsp/lspResolver.ts index 25fc767daaa..faecf7e2dab 100644 --- a/packages/core/src/shared/lsp/lspResolver.ts +++ b/packages/core/src/shared/lsp/lspResolver.ts @@ -226,7 +226,7 @@ export class LanguageServerResolver { res: await new HttpResourceFetcher(content.url, { showUrl: true, timeout: timeout, - swallowError: false, + 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 1801f4a7b7c..d013966d0a3 100644 --- a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts +++ b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts @@ -22,7 +22,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.swallowError True if we want to swallow the request error, false if we want the error to keep bubbling up + * @param {boolean} params.throwOnError True if we want to throw if there's request error, defaul to false */ public constructor( private readonly url: string, @@ -30,10 +30,10 @@ export class HttpResourceFetcher implements ResourceFetcher { showUrl: boolean friendlyName?: string timeout?: Timeout - swallowError?: boolean + throwOnError?: boolean } ) { - this.params.swallowError = this.params.swallowError ?? true + this.params.throwOnError = this.params.throwOnError ?? false } /** @@ -86,10 +86,10 @@ export class HttpResourceFetcher implements ResourceFetcher { `Error downloading ${this.logText()}: %s`, error.message ?? error.code ?? error.response.statusText ?? error.response.status ) - if (this.params.swallowError) { - return undefined + if (this.params.throwOnError) { + throw error } - throw error + return undefined } } From ee347a9451adf97051c8942e0cf2275e9ec85952 Mon Sep 17 00:00:00 2001 From: tomzu Date: Mon, 3 Mar 2025 13:07:30 -0500 Subject: [PATCH 7/7] wrap request cancelled error in isUserCancelledError --- packages/core/src/shared/errors.ts | 7 ++++++- packages/core/src/shared/lsp/lspResolver.ts | 8 +------- .../src/shared/resourcefetcher/httpResourceFetcher.ts | 7 ++++--- 3 files changed, 11 insertions(+), 11 deletions(-) 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 faecf7e2dab..80ad2fe0b72 100644 --- a/packages/core/src/shared/lsp/lspResolver.ts +++ b/packages/core/src/shared/lsp/lspResolver.ts @@ -15,8 +15,7 @@ import { createHash } from '../crypto' import { lspSetupStage, StageResolver, tryStageResolvers } from './utils/setupStage' import { HttpResourceFetcher } from '../resourcefetcher/httpResourceFetcher' import { showMessageWithCancel } from '../../shared/utilities/messages' -import { CancellationError, Timeout } from '../utilities/timeoutUtils' -import { RequestCancelledError } from '../request' +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 @@ -114,11 +113,6 @@ export class LanguageServerResolver { } else { throw new ToolkitError('Failed to download server from remote', { code: 'RemoteDownloadFailed' }) } - } catch (err) { - if (err instanceof RequestCancelledError) { - throw new CancellationError('user') - } - throw err } finally { timeout.dispose() } diff --git a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts index d013966d0a3..18cc696770f 100644 --- a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts +++ b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts @@ -7,7 +7,8 @@ import { VSCODE_EXTENSION_ID } from '../extensions' import { getLogger, Logger } from '../logger/logger' import { ResourceFetcher } from './resourcefetcher' import { Timeout, CancelEvent, waitUntil } from '../utilities/timeoutUtils' -import request, { RequestCancelledError, RequestError } from '../request' +import request, { RequestError } from '../request' +import { isUserCancelledError } from '../errors' type RequestHeaders = { eTag?: string; gZip?: boolean } @@ -120,8 +121,8 @@ export class HttpResourceFetcher implements ResourceFetcher { interval: 100, backoff: 2, retryOnFail: (error: Error) => { - // retry unless we got an user cancellation error - return !(error instanceof RequestCancelledError) + // Retry unless the user intentionally canceled the operation. + return !isUserCancelledError(error) }, } )