From db94de8d9b2b8c671b3082185fbe4948f6430f48 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Wed, 22 Nov 2023 17:37:04 -0500 Subject: [PATCH] Adds cancellation & timeout to fetch requests Adds default (60s) timeout to fetch requests Refs #3013 --- .eslintrc.base.json | 5 ++ src/env/node/git/git.ts | 6 +- src/env/node/git/localGitProvider.ts | 12 ++- src/errors.ts | 3 +- src/plus/drafts/draftsService.ts | 4 +- src/plus/gk/account/authenticationConnection.ts | 4 +- src/plus/gk/account/authenticationProvider.ts | 7 +- src/plus/gk/account/subscriptionService.ts | 4 +- src/plus/gk/serverConnection.ts | 96 ++++++++++++++++-------- src/plus/integrations/providerIntegration.ts | 4 +- src/plus/integrations/providers/github/github.ts | 2 +- src/plus/integrations/providers/gitlab/gitlab.ts | 2 +- 12 files changed, 96 insertions(+), 53 deletions(-) diff --git a/.eslintrc.base.json b/.eslintrc.base.json index a5c13d8..48a8d93 100644 --- a/.eslintrc.base.json +++ b/.eslintrc.base.json @@ -96,6 +96,11 @@ "group": ["react-dom"], "importNames": ["Container"], "message": "Use our Container instead" + }, + { + "group": ["vscode"], + "importNames": ["CancellationError"], + "message": "Use our CancellationError instead" } ] } diff --git a/src/env/node/git/git.ts b/src/env/node/git/git.ts index e4a2816..f6f3899 100644 --- a/src/env/node/git/git.ts +++ b/src/env/node/git/git.ts @@ -284,9 +284,9 @@ export class Git { } if (cancellation) { - const controller = new AbortController(); - spawnOpts.signal = controller.signal; - cancellation.onCancellationRequested(() => controller.abort()); + const aborter = new AbortController(); + spawnOpts.signal = aborter.signal; + cancellation.onCancellationRequested(() => aborter.abort()); } const proc = spawn(await this.path(), args, spawnOpts); diff --git a/src/env/node/git/localGitProvider.ts b/src/env/node/git/localGitProvider.ts index 15aaea8..0592100 100644 --- a/src/env/node/git/localGitProvider.ts +++ b/src/env/node/git/localGitProvider.ts @@ -648,13 +648,11 @@ export class LocalGitProvider implements GitProvider, Disposable { // Check if the url returns a 200 status code let promise = this._pendingRemoteVisibility.get(url); if (promise == null) { - const cancellation = new AbortController(); - let timeout: ReturnType; - promise = fetch(url, { method: 'HEAD', agent: getProxyAgent(), signal: cancellation.signal }).then(r => { - clearTimeout(timeout); - return r; - }); - timeout = setTimeout(() => cancellation.abort(), 30000); + const aborter = new AbortController(); + const timer = setTimeout(() => aborter.abort(), 30000); + + promise = fetch(url, { method: 'HEAD', agent: getProxyAgent(), signal: aborter.signal }); + void promise.finally(() => clearTimeout(timer)); this._pendingRemoteVisibility.set(url, promise); } diff --git a/src/errors.ts b/src/errors.ts index 0708da0..74ff4f6 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -1,4 +1,5 @@ import type { Uri } from 'vscode'; +// eslint-disable-next-line no-restricted-imports import { CancellationError as _CancellationError } from 'vscode'; import type { Response } from '@env/fetch'; import type { RequiredSubscriptionPlans, Subscription } from './plus/gk/account/subscription'; @@ -95,7 +96,7 @@ export class AuthenticationRequiredError extends Error { } export class CancellationError extends _CancellationError { - constructor() { + constructor(public readonly original?: Error) { super(); Error.captureStackTrace?.(this, CancellationError); diff --git a/src/plus/drafts/draftsService.ts b/src/plus/drafts/draftsService.ts index c7d52de..409fb66 100644 --- a/src/plus/drafts/draftsService.ts +++ b/src/plus/drafts/draftsService.ts @@ -121,7 +121,7 @@ export class DraftService implements Disposable { const files = diffFiles?.files.map(f => ({ ...f, gkRepositoryId: patch.gitRepositoryId })) ?? []; // Upload patch to returned S3 url - await this.connection.fetchRaw(url, { + await this.connection.fetch(url, { method: method, headers: { 'Content-Type': 'text/plain', @@ -528,7 +528,7 @@ export class DraftService implements Disposable { const { url, method, headers } = secureLink; // Download patch from returned S3 url - const contentsRsp = await this.connection.fetchRaw(url, { + const contentsRsp = await this.connection.fetch(url, { method: method, headers: { Accept: 'text/plain', diff --git a/src/plus/gk/account/authenticationConnection.ts b/src/plus/gk/account/authenticationConnection.ts index 25b6cd2..f20f584 100644 --- a/src/plus/gk/account/authenticationConnection.ts +++ b/src/plus/gk/account/authenticationConnection.ts @@ -38,13 +38,13 @@ export class AuthenticationConnection implements Disposable { return new Promise(resolve => setTimeout(resolve, 50)); } - @debug({ args: false }) + @debug({ args: false, exit: r => `returned ${r.id}` }) async getAccountInfo(token: string): Promise { const scope = getLogScope(); let rsp: Response; try { - rsp = await this.connection.fetchApi('user', undefined, token); + rsp = await this.connection.fetchApi('user', undefined, { token: token }); } catch (ex) { Logger.error(ex, scope); throw ex; diff --git a/src/plus/gk/account/authenticationProvider.ts b/src/plus/gk/account/authenticationProvider.ts index 17b3eb6..2deacfd 100644 --- a/src/plus/gk/account/authenticationProvider.ts +++ b/src/plus/gk/account/authenticationProvider.ts @@ -6,6 +6,7 @@ import type { import { authentication, Disposable, EventEmitter, window } from 'vscode'; import { uuid } from '@env/crypto'; import type { Container, Environment } from '../../../container'; +import { CancellationError } from '../../../errors'; import { debug } from '../../../system/decorators/log'; import { Logger } from '../../../system/logger'; import { getLogScope, setLogScopeExit } from '../../../system/logger.scope'; @@ -96,7 +97,11 @@ export class AccountAuthenticationProvider implements AuthenticationProvider, Di if (ex === 'Cancelled') throw ex; Logger.error(ex, scope); - void window.showErrorMessage(`Unable to sign in to GitKraken: ${ex}`); + void window.showErrorMessage( + `Unable to sign in to GitKraken: ${ + ex instanceof CancellationError ? 'request timed out' : ex + }. Please try again. If this issue persists, please contact support.`, + ); throw ex; } } diff --git a/src/plus/gk/account/subscriptionService.ts b/src/plus/gk/account/subscriptionService.ts index 54e4e3e..930cb29 100644 --- a/src/plus/gk/account/subscriptionService.ts +++ b/src/plus/gk/account/subscriptionService.ts @@ -363,7 +363,7 @@ export class SubscriptionService implements Disposable { method: 'POST', body: JSON.stringify({ id: session.account.id }), }, - session.accessToken, + { token: session.accessToken }, ); if (!rsp.ok) { @@ -570,7 +570,7 @@ export class SubscriptionService implements Disposable { method: 'POST', body: JSON.stringify(checkInData), }, - session.accessToken, + { token: session.accessToken }, ); if (!rsp.ok) { diff --git a/src/plus/gk/serverConnection.ts b/src/plus/gk/serverConnection.ts index 9e9d04b..9fef382 100644 --- a/src/plus/gk/serverConnection.ts +++ b/src/plus/gk/serverConnection.ts @@ -1,13 +1,22 @@ -import type { Disposable } from 'vscode'; +import type { CancellationToken, Disposable } from 'vscode'; import { Uri } from 'vscode'; import type { RequestInfo, RequestInit, Response } from '@env/fetch'; import { fetch as _fetch, getProxyAgent } from '@env/fetch'; import type { Container } from '../../container'; -import { AuthenticationRequiredError } from '../../errors'; +import { AuthenticationRequiredError, CancellationError } from '../../errors'; import { memoize } from '../../system/decorators/memoize'; import { Logger } from '../../system/logger'; import { getLogScope } from '../../system/logger.scope'; +interface FetchOptions { + cancellation?: CancellationToken; + timeout?: number; +} + +interface GKFetchOptions extends FetchOptions { + token?: string; +} + export class ServerConnection implements Disposable { constructor(private readonly container: Container) {} @@ -96,60 +105,85 @@ export class ServerConnection implements Disposable { return 'Visual-Studio-Code-GitLens'; } - async fetch(url: RequestInfo, init?: RequestInit, token?: string): Promise { + async fetch(url: RequestInfo, init?: RequestInit, options?: FetchOptions): Promise { const scope = getLogScope(); + if (options?.cancellation?.isCancellationRequested) throw new CancellationError(); + + const aborter = new AbortController(); + + let timeout; + if (options?.cancellation != null) { + timeout = options.timeout; // Don't set a default timeout if we have a cancellation token + options.cancellation.onCancellationRequested(() => aborter.abort()); + } else { + timeout = options?.timeout ?? 60 * 1000; + } + + const timer = timeout != null ? setTimeout(() => aborter.abort(), timeout) : undefined; + try { - token ??= await this.getAccessToken(); - const options = { + const promise = _fetch(url, { agent: getProxyAgent(), ...init, headers: { - Authorization: `Bearer ${token}`, 'User-Agent': this.userAgent, - 'Content-Type': 'application/json', ...init?.headers, }, - }; - - // TODO@eamodio handle common response errors - - return await _fetch(url, options); + signal: aborter?.signal, + }); + void promise.finally(() => clearTimeout(timer)); + return await promise; } catch (ex) { Logger.error(ex, scope); + if (ex.name === 'AbortError') throw new CancellationError(ex); + throw ex; } } - async fetchApi(path: string, init?: RequestInit, token?: string): Promise { - return this.fetch(this.getApiUrl(path), init, token); + async fetchApi(path: string, init?: RequestInit, options?: GKFetchOptions): Promise { + return this.gkFetch(this.getApiUrl(path), init, options); } - async fetchApiGraphQL(path: string, request: GraphQLRequest, init?: RequestInit) { - return this.fetchApi(path, { - method: 'POST', - ...init, - body: JSON.stringify(request), - }); + async fetchApiGraphQL(path: string, request: GraphQLRequest, init?: RequestInit, options?: GKFetchOptions) { + return this.fetchApi( + path, + { + method: 'POST', + ...init, + body: JSON.stringify(request), + }, + options, + ); } - async fetchGkDevApi(path: string, init?: RequestInit, token?: string): Promise { - return this.fetch(this.getGkDevApiUrl(path), init, token); + async fetchGkDevApi(path: string, init?: RequestInit, options?: GKFetchOptions): Promise { + return this.gkFetch(this.getGkDevApiUrl(path), init, options); } - async fetchRaw(url: RequestInfo, init?: RequestInit): Promise { + private async gkFetch(url: RequestInfo, init?: RequestInit, options?: GKFetchOptions): Promise { const scope = getLogScope(); try { - const options = { - agent: getProxyAgent(), - ...init, - headers: { - 'User-Agent': this.userAgent, - ...init?.headers, + let token; + ({ token, ...options } = options ?? {}); + token ??= await this.getAccessToken(); + + // TODO@eamodio handle common response errors + + return this.fetch( + url, + { + ...init, + headers: { + Authorization: `Bearer ${token}`, + 'Content-Type': 'application/json', + ...init?.headers, + }, }, - }; - return await _fetch(url, options); + options, + ); } catch (ex) { Logger.error(ex, scope); throw ex; diff --git a/src/plus/integrations/providerIntegration.ts b/src/plus/integrations/providerIntegration.ts index 12af3dc..32de6e2 100644 --- a/src/plus/integrations/providerIntegration.ts +++ b/src/plus/integrations/providerIntegration.ts @@ -1,7 +1,7 @@ import type { AuthenticationSession, CancellationToken, Event, MessageItem } from 'vscode'; -import { CancellationError, EventEmitter, window } from 'vscode'; +import { EventEmitter, window } from 'vscode'; import type { Container } from '../../container'; -import { AuthenticationError, ProviderRequestClientError } from '../../errors'; +import { AuthenticationError, CancellationError, ProviderRequestClientError } from '../../errors'; import type { PagedResult } from '../../git/gitProvider'; import type { Account } from '../../git/models/author'; import type { DefaultBranch } from '../../git/models/defaultBranch'; diff --git a/src/plus/integrations/providers/github/github.ts b/src/plus/integrations/providers/github/github.ts index 2f93025..f663961 100644 --- a/src/plus/integrations/providers/github/github.ts +++ b/src/plus/integrations/providers/github/github.ts @@ -2463,7 +2463,7 @@ export class GitHubApi implements Disposable { ex: RequestError | (Error & { name: 'AbortError' }), scope: LogScope | undefined, ): void { - if (ex.name === 'AbortError') throw new CancellationError(); + if (ex.name === 'AbortError') throw new CancellationError(ex); switch (ex.status) { case 404: // Not found diff --git a/src/plus/integrations/providers/gitlab/gitlab.ts b/src/plus/integrations/providers/gitlab/gitlab.ts index 700d6e6..586eb80 100644 --- a/src/plus/integrations/providers/gitlab/gitlab.ts +++ b/src/plus/integrations/providers/gitlab/gitlab.ts @@ -879,7 +879,7 @@ $search: String! ex: ProviderFetchError | (Error & { name: 'AbortError' }), scope: LogScope | undefined, ): void { - if (ex.name === 'AbortError' || !(ex instanceof ProviderFetchError)) throw new CancellationError(); + if (ex.name === 'AbortError' || !(ex instanceof ProviderFetchError)) throw new CancellationError(ex); switch (ex.status) { case 404: // Not found