-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Exp enable completions and NES fetcher #1245
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,19 +5,28 @@ | |||||||||||||||||
|
||||||||||||||||||
import { Readable } from 'stream'; | ||||||||||||||||||
import { ILogService } from '../../log/common/logService'; | ||||||||||||||||||
import { FetchOptions, Response } from '../common/fetcherService'; | ||||||||||||||||||
import { FetcherId, FetchOptions, Response } from '../common/fetcherService'; | ||||||||||||||||||
import { IFetcher } from '../common/networking'; | ||||||||||||||||||
import { Config, ConfigKey, IConfigurationService } from '../../configuration/common/configurationService'; | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
export async function fetchWithFallbacks(availableFetchers: readonly IFetcher[], url: string, options: FetchOptions, logService: ILogService): Promise<{ response: Response; updatedFetchers?: IFetcher[] }> { | ||||||||||||||||||
const fetcherConfigKeys: Record<FetcherId, Config<boolean>> = { | ||||||||||||||||||
'electron-fetch': ConfigKey.Shared.DebugUseElectronFetcher, | ||||||||||||||||||
'node-fetch': ConfigKey.Shared.DebugUseNodeFetchFetcher, | ||||||||||||||||||
'node-http': ConfigKey.Shared.DebugUseNodeFetcher, | ||||||||||||||||||
}; | ||||||||||||||||||
|
||||||||||||||||||
export async function fetchWithFallbacks(availableFetchers: readonly IFetcher[], url: string, options: FetchOptions, knownBadFetchers: Set<string>, configurationService: IConfigurationService, logService: ILogService): Promise<{ response: Response; updatedFetchers?: IFetcher[]; updatedKnownBadFetchers?: Set<string> }> { | ||||||||||||||||||
if (options.retryFallbacks && availableFetchers.length > 1) { | ||||||||||||||||||
let firstResult: { ok: boolean; response: Response } | { ok: false; err: any } | undefined; | ||||||||||||||||||
const updatedKnownBadFetchers = new Set<string>(); | ||||||||||||||||||
Comment on lines
+19
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The parameter and return types use Set while the codebase introduces FetcherId; narrowing these to Set improves type safety and consistency with other parts (tests already use Set).
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||
for (const fetcher of availableFetchers) { | ||||||||||||||||||
const result = await tryFetch(fetcher, url, options, logService); | ||||||||||||||||||
if (fetcher === availableFetchers[0]) { | ||||||||||||||||||
firstResult = result; | ||||||||||||||||||
} | ||||||||||||||||||
if (!result.ok) { | ||||||||||||||||||
updatedKnownBadFetchers.add(fetcher.getUserAgentLibrary()); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updatedKnownBadFetchers is initialized empty instead of cloning/unioning the incoming knownBadFetchers, and the caller later replaces its state (see FetcherService) leading to loss of previously known bad fetchers; either (a) initialize with new Set(knownBadFetchers) and add new ones, or (b) return only newly failed fetchers but have the caller union rather than replace to ensure consistent accumulation semantics. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||
continue; | ||||||||||||||||||
} | ||||||||||||||||||
if (fetcher !== availableFetchers[0]) { | ||||||||||||||||||
|
@@ -29,7 +38,7 @@ export async function fetchWithFallbacks(availableFetchers: readonly IFetcher[], | |||||||||||||||||
const updatedFetchers = availableFetchers.slice(); | ||||||||||||||||||
updatedFetchers.splice(updatedFetchers.indexOf(fetcher), 1); | ||||||||||||||||||
updatedFetchers.unshift(fetcher); | ||||||||||||||||||
return { response: result.response, updatedFetchers }; | ||||||||||||||||||
return { response: result.response, updatedFetchers, updatedKnownBadFetchers }; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updatedKnownBadFetchers is initialized empty instead of cloning/unioning the incoming knownBadFetchers, and the caller later replaces its state (see FetcherService) leading to loss of previously known bad fetchers; either (a) initialize with new Set(knownBadFetchers) and add new ones, or (b) return only newly failed fetchers but have the caller union rather than replace to ensure consistent accumulation semantics. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||
} | ||||||||||||||||||
return { response: result.response }; | ||||||||||||||||||
} | ||||||||||||||||||
|
@@ -38,12 +47,23 @@ export async function fetchWithFallbacks(availableFetchers: readonly IFetcher[], | |||||||||||||||||
} | ||||||||||||||||||
throw firstResult!.err; | ||||||||||||||||||
} | ||||||||||||||||||
const fetcher = options.useFetcher && availableFetchers.find(f => f.getUserAgentLibrary() === options.useFetcher) || availableFetchers[0]; | ||||||||||||||||||
let fetcher = availableFetchers[0]; | ||||||||||||||||||
if (options.useFetcher) { | ||||||||||||||||||
if (options.useFetcher === fetcher.getUserAgentLibrary()) { | ||||||||||||||||||
logService.debug(`FetcherService: using ${options.useFetcher} as requested.`); | ||||||||||||||||||
if (knownBadFetchers.has(options.useFetcher)) { | ||||||||||||||||||
logService.trace(`FetcherService: not using requested fetcher ${options.useFetcher} as it is known to be failing, using ${fetcher.getUserAgentLibrary()} instead.`); | ||||||||||||||||||
} else { | ||||||||||||||||||
logService.info(`FetcherService: could not find requested fetcher ${options.useFetcher}, using ${fetcher.getUserAgentLibrary()} instead.`); | ||||||||||||||||||
const configKey = fetcherConfigKeys[options.useFetcher]; | ||||||||||||||||||
if (configKey && configurationService.inspectConfig(configKey)?.globalValue === false) { | ||||||||||||||||||
logService.trace(`FetcherService: not using requested fetcher ${options.useFetcher} as it is disabled in user settings, using ${fetcher.getUserAgentLibrary()} instead.`); | ||||||||||||||||||
} else { | ||||||||||||||||||
const requestedFetcher = availableFetchers.find(f => f.getUserAgentLibrary() === options.useFetcher); | ||||||||||||||||||
if (requestedFetcher) { | ||||||||||||||||||
fetcher = requestedFetcher; | ||||||||||||||||||
logService.trace(`FetcherService: using ${options.useFetcher} as requested.`); | ||||||||||||||||||
} else { | ||||||||||||||||||
logService.info(`FetcherService: could not find requested fetcher ${options.useFetcher}, using ${fetcher.getUserAgentLibrary()} instead.`); | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
return { response: await fetcher.fetch(url, options) }; | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -18,6 +18,7 @@ export class FetcherService implements IFetcherService { | |||||||||
|
||||||||||
declare readonly _serviceBrand: undefined; | ||||||||||
private _availableFetchers: readonly IFetcher[] | undefined; | ||||||||||
private _knownBadFetchers = new Set<string>(); | ||||||||||
private _experimentationService: IExperimentationService | undefined; | ||||||||||
|
||||||||||
constructor( | ||||||||||
|
@@ -88,10 +89,13 @@ export class FetcherService implements IFetcherService { | |||||||||
} | ||||||||||
|
||||||||||
async fetch(url: string, options: FetchOptions): Promise<Response> { | ||||||||||
const { response: res, updatedFetchers } = await fetchWithFallbacks(this._getAvailableFetchers(), url, options, this._logService); | ||||||||||
const { response: res, updatedFetchers, updatedKnownBadFetchers } = await fetchWithFallbacks(this._getAvailableFetchers(), url, options, this._knownBadFetchers, this._configurationService, this._logService); | ||||||||||
if (updatedFetchers) { | ||||||||||
this._availableFetchers = updatedFetchers; | ||||||||||
} | ||||||||||
if (updatedKnownBadFetchers) { | ||||||||||
this._knownBadFetchers = updatedKnownBadFetchers; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replacing the existing this._knownBadFetchers set with updatedKnownBadFetchers discards previously known bad fetchers on calls where only new failures are surfaced; this causes earlier failing fetchers to be forgotten unless they fail again in the same run. Merge instead: for (const f of updatedKnownBadFetchers) this._knownBadFetchers.add(f); to preserve cumulative state.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||
} | ||||||||||
return res; | ||||||||||
} | ||||||||||
|
||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updatedKnownBadFetchers is initialized empty instead of cloning/unioning the incoming knownBadFetchers, and the caller later replaces its state (see FetcherService) leading to loss of previously known bad fetchers; either (a) initialize with new Set(knownBadFetchers) and add new ones, or (b) return only newly failed fetchers but have the caller union rather than replace to ensure consistent accumulation semantics.
Copilot uses AI. Check for mistakes.