From 373b0d9ce88f6e170c876c838e6ca38827918885 Mon Sep 17 00:00:00 2001 From: Josh Pinkney Date: Mon, 20 Jan 2025 12:01:25 -0500 Subject: [PATCH 1/3] refactor(core): Manifest/LSP fetching should use httpResourceFetcher --- packages/core/src/shared/lsp/lspResolver.ts | 9 ++-- .../core/src/shared/lsp/manifestResolver.ts | 15 +++---- .../resourcefetcher/httpResourceFetcher.ts | 44 ------------------- 3 files changed, 12 insertions(+), 56 deletions(-) diff --git a/packages/core/src/shared/lsp/lspResolver.ts b/packages/core/src/shared/lsp/lspResolver.ts index 7a6fc4102b0..7613a06a464 100644 --- a/packages/core/src/shared/lsp/lspResolver.ts +++ b/packages/core/src/shared/lsp/lspResolver.ts @@ -12,7 +12,9 @@ import AdmZip from 'adm-zip' import { TargetContent, logger, LspResult, LspVersion, Manifest } from './types' import { getApplicationSupportFolder } from '../vscode/env' import { createHash } from '../crypto' -import request from '../request' +import { HttpResourceFetcher } from '../resourcefetcher/httpResourceFetcher' + +const lspFetchRetries = 3 export class LanguageServerResolver { constructor( @@ -165,9 +167,8 @@ export class LanguageServerResolver { } const downloadTasks = contents.map(async (content) => { - // TODO This should be using the retryable http library but it doesn't seem to support zips right now - const res = await request.fetch('GET', content.url).response - if (!res.ok || !res.body) { + const res = await new HttpResourceFetcher(content.url, { showUrl: true, retries: lspFetchRetries }).get() + if (!res || !res.ok || !res.body) { return false } diff --git a/packages/core/src/shared/lsp/manifestResolver.ts b/packages/core/src/shared/lsp/manifestResolver.ts index 36685890620..0f5a8386693 100644 --- a/packages/core/src/shared/lsp/manifestResolver.ts +++ b/packages/core/src/shared/lsp/manifestResolver.ts @@ -5,10 +5,10 @@ import { getLogger } from '../logger/logger' import { ToolkitError } from '../errors' -import { RetryableResourceFetcher } from '../resourcefetcher/httpResourceFetcher' import { Timeout } from '../utilities/timeoutUtils' import globals from '../extensionGlobals' import { Manifest } from './types' +import { HttpResourceFetcher } from '../resourcefetcher/httpResourceFetcher' const logger = getLogger('lsp') @@ -21,6 +21,7 @@ type ManifestStorage = Record const manifestStorageKey = 'aws.toolkit.lsp.manifest' const manifestTimeoutMs = 15000 +const manifestFetchRetries = 3 export class ManifestResolver { constructor( @@ -40,14 +41,12 @@ export class ManifestResolver { } private async fetchRemoteManifest(): Promise { - const resourceFetcher = new RetryableResourceFetcher({ - resource: this.manifestURL, - params: { - timeout: new Timeout(manifestTimeoutMs), - }, - }) + const resp = await new HttpResourceFetcher(this.manifestURL, { + showUrl: true, + timeout: new Timeout(manifestTimeoutMs), + retries: manifestFetchRetries, + }).getNewETagContent(this.getEtag()) - const resp = await resourceFetcher.getNewETagContent(this.getEtag()) if (!resp.content) { throw new ToolkitError('New content was not downloaded; fallback to the locally stored manifest') } diff --git a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts index ae1cb76fb77..e85e1ded70b 100644 --- a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts +++ b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts @@ -134,50 +134,6 @@ export class HttpResourceFetcher implements ResourceFetcher { } } -export class RetryableResourceFetcher extends HttpResourceFetcher { - private readonly retryNumber: number - private readonly retryIntervalMs: number - private readonly resource: string - - constructor({ - resource, - params: { retryNumber = 5, retryIntervalMs = 3000, showUrl = true, timeout = new Timeout(5000) }, - }: { - resource: string - params: { - retryNumber?: number - retryIntervalMs?: number - showUrl?: boolean - timeout?: Timeout - } - }) { - super(resource, { - showUrl, - timeout, - }) - this.retryNumber = retryNumber - this.retryIntervalMs = retryIntervalMs - this.resource = resource - } - - fetch(versionTag?: string) { - return withRetries( - async () => { - try { - return await this.getNewETagContent(versionTag) - } catch (err) { - getLogger('lsp').error('Failed to fetch at endpoint: %s, err: %s', this.resource, err) - throw err - } - }, - { - maxRetries: this.retryNumber, - delay: this.retryIntervalMs, - } - ) - } -} - /** * Retrieves JSON property value from a remote resource * @param property property to retrieve From fdb89cbbc12a492e867ca292cee985b61b2fd109 Mon Sep 17 00:00:00 2001 From: Josh Pinkney Date: Fri, 24 Jan 2025 09:48:52 -0500 Subject: [PATCH 2/3] fixup --- packages/webpack.web.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/webpack.web.config.js b/packages/webpack.web.config.js index fc6ca86d1d9..0f82af67389 100644 --- a/packages/webpack.web.config.js +++ b/packages/webpack.web.config.js @@ -41,7 +41,7 @@ module.exports = (env, argv) => { * environments. The following allows compilation to pass in Web mode by never bundling the module in the final output for web mode. */ new webpack.IgnorePlugin({ - resourceRegExp: /httpResourceFetcher/, // matches the path in the require() statement + resourceRegExp: /node\/httpResourceFetcher/, // matches the path in the require() statement }), /** * HACK: the ps-list module breaks Web mode if imported, BUT we still dynamically import this module for non web mode From e74f3c78010e80547175e8815cd3df83bb049092 Mon Sep 17 00:00:00 2001 From: Josh Pinkney Date: Fri, 24 Jan 2025 11:16:00 -0500 Subject: [PATCH 3/3] fixup --- packages/core/src/shared/lsp/lspResolver.ts | 4 +--- packages/core/src/shared/lsp/manifestResolver.ts | 2 -- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/core/src/shared/lsp/lspResolver.ts b/packages/core/src/shared/lsp/lspResolver.ts index 7613a06a464..4959f675986 100644 --- a/packages/core/src/shared/lsp/lspResolver.ts +++ b/packages/core/src/shared/lsp/lspResolver.ts @@ -14,8 +14,6 @@ import { getApplicationSupportFolder } from '../vscode/env' import { createHash } from '../crypto' import { HttpResourceFetcher } from '../resourcefetcher/httpResourceFetcher' -const lspFetchRetries = 3 - export class LanguageServerResolver { constructor( private readonly manifest: Manifest, @@ -167,7 +165,7 @@ export class LanguageServerResolver { } const downloadTasks = contents.map(async (content) => { - const res = await new HttpResourceFetcher(content.url, { showUrl: true, retries: lspFetchRetries }).get() + const res = await new HttpResourceFetcher(content.url, { showUrl: true }).get() if (!res || !res.ok || !res.body) { return false } diff --git a/packages/core/src/shared/lsp/manifestResolver.ts b/packages/core/src/shared/lsp/manifestResolver.ts index 0f5a8386693..0cf27b1293b 100644 --- a/packages/core/src/shared/lsp/manifestResolver.ts +++ b/packages/core/src/shared/lsp/manifestResolver.ts @@ -21,7 +21,6 @@ type ManifestStorage = Record const manifestStorageKey = 'aws.toolkit.lsp.manifest' const manifestTimeoutMs = 15000 -const manifestFetchRetries = 3 export class ManifestResolver { constructor( @@ -44,7 +43,6 @@ export class ManifestResolver { const resp = await new HttpResourceFetcher(this.manifestURL, { showUrl: true, timeout: new Timeout(manifestTimeoutMs), - retries: manifestFetchRetries, }).getNewETagContent(this.getEtag()) if (!resp.content) {