diff --git a/packages/core/src/notifications/controller.ts b/packages/core/src/notifications/controller.ts index 83886a14c1a..41ec81f8500 100644 --- a/packages/core/src/notifications/controller.ts +++ b/packages/core/src/notifications/controller.ts @@ -24,11 +24,11 @@ import { NotificationsNode } from './panelNode' import { Commands } from '../shared/vscode/commands2' import { RuleEngine } from './rules' import { TreeNode } from '../shared/treeview/resourceTreeDataProvider' -import { withRetries } from '../shared/utilities/functionUtils' import { FileResourceFetcher } from '../shared/resourcefetcher/fileResourceFetcher' import { isAmazonQ } from '../shared/extensionUtilities' import { telemetry } from '../shared/telemetry/telemetry' import { randomUUID } from '../shared/crypto' +import { waitUntil } from '../shared/utilities/timeoutUtils' const logger = getLogger('notifications') @@ -266,8 +266,8 @@ export interface NotificationFetcher { } export class RemoteFetcher implements NotificationFetcher { - public static readonly retryNumber = 5 public static readonly retryIntervalMs = 30000 + public static readonly retryTimeout = RemoteFetcher.retryIntervalMs * 5 private readonly startUpEndpoint: string = 'https://idetoolkits-hostedfiles.amazonaws.com/Notifications/VSCode/startup/1.x.json' @@ -286,7 +286,7 @@ export class RemoteFetcher implements NotificationFetcher { }) logger.verbose('Attempting to fetch notifications for category: %s at endpoint: %s', category, endpoint) - return withRetries( + return waitUntil( async () => { try { return await fetcher.getNewETagContent(versionTag) @@ -296,8 +296,9 @@ export class RemoteFetcher implements NotificationFetcher { } }, { - maxRetries: RemoteFetcher.retryNumber, - delay: RemoteFetcher.retryIntervalMs, + interval: RemoteFetcher.retryIntervalMs, + timeout: RemoteFetcher.retryTimeout, + retryOnFail: true, // No exponential backoff - necessary? } ) diff --git a/packages/core/src/shared/crashMonitoring.ts b/packages/core/src/shared/crashMonitoring.ts index 5638ad875f0..662a7907875 100644 --- a/packages/core/src/shared/crashMonitoring.ts +++ b/packages/core/src/shared/crashMonitoring.ts @@ -17,8 +17,8 @@ import fs from './fs/fs' import { getLogger } from './logger/logger' import { crashMonitoringDirName } from './constants' import { throwOnUnstableFileSystem } from './filesystemUtilities' -import { withRetries } from './utilities/functionUtils' import { truncateUuid } from './crypto' +import { waitUntil } from './utilities/timeoutUtils' const className = 'CrashMonitoring' @@ -489,7 +489,12 @@ export class FileSystemState { this.deps.devLogger?.debug(`HEARTBEAT sent for ${truncateUuid(this.ext.sessionId)}`) } const funcWithCtx = () => withFailCtx('sendHeartbeatState', func) - const funcWithRetries = withRetries(funcWithCtx, { maxRetries: 6, delay: 100, backoff: 2 }) + const funcWithRetries = waitUntil(funcWithCtx, { + timeout: 15_000, + interval: 100, + backoff: 2, + retryOnFail: true, + }) return funcWithRetries } catch (e) { @@ -542,7 +547,12 @@ export class FileSystemState { await nodeFs.rm(filePath, { force: true }) } const funcWithCtx = () => withFailCtx(ctx, func) - const funcWithRetries = withRetries(funcWithCtx, { maxRetries: 6, delay: 100, backoff: 2 }) + const funcWithRetries = waitUntil(funcWithCtx, { + timeout: 15_000, + interval: 100, + backoff: 2, + retryOnFail: true, + }) await funcWithRetries } @@ -609,7 +619,7 @@ export class FileSystemState { } const funcWithIgnoreBadFile = () => ignoreBadFileError(loadExtFromDisk) const funcWithRetries = () => - withRetries(funcWithIgnoreBadFile, { maxRetries: 6, delay: 100, backoff: 2 }) + waitUntil(funcWithIgnoreBadFile, { timeout: 15_000, interval: 100, backoff: 2, retryOnFail: true }) const funcWithCtx = () => withFailCtx('parseRunningExtFile', funcWithRetries) const ext: ExtInstanceHeartbeat | undefined = await funcWithCtx() diff --git a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts index e942e0ec2d0..e85e1ded70b 100644 --- a/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts +++ b/packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts @@ -6,9 +6,8 @@ import { VSCODE_EXTENSION_ID } from '../extensions' import { getLogger, Logger } from '../logger' import { ResourceFetcher } from './resourcefetcher' -import { Timeout, CancelEvent } from '../utilities/timeoutUtils' +import { Timeout, CancelEvent, waitUntil } from '../utilities/timeoutUtils' import request, { RequestError } from '../request' -import { withRetries } from '../utilities/functionUtils' type RequestHeaders = { eTag?: string; gZip?: boolean } @@ -30,7 +29,6 @@ export class HttpResourceFetcher implements ResourceFetcher { showUrl: boolean friendlyName?: string timeout?: Timeout - retries?: number } ) {} @@ -97,7 +95,7 @@ export class HttpResourceFetcher implements ResourceFetcher { } private async getResponseFromGetRequest(timeout?: Timeout, headers?: RequestHeaders): Promise { - return withRetries( + return waitUntil( () => { const req = request.fetch('GET', this.url, { headers: this.buildRequestHeaders(headers), @@ -111,7 +109,10 @@ export class HttpResourceFetcher implements ResourceFetcher { return req.response.finally(() => cancelListener?.dispose()) }, { - maxRetries: this.params.retries ?? 1, + timeout: 3000, + interval: 100, + backoff: 2, + retryOnFail: true, } ) } diff --git a/packages/core/src/shared/utilities/functionUtils.ts b/packages/core/src/shared/utilities/functionUtils.ts index f61a3abde34..d21727bac1d 100644 --- a/packages/core/src/shared/utilities/functionUtils.ts +++ b/packages/core/src/shared/utilities/functionUtils.ts @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { sleep, Timeout } from './timeoutUtils' +import { Timeout } from './timeoutUtils' /** * Creates a function that always returns a 'shared' Promise. @@ -145,34 +145,3 @@ export function cancellableDebounce( cancel: cancel, } } - -/** - * Executes the given function, retrying if it throws. - * - * @param opts - if no opts given, defaults are used - */ -export async function withRetries( - fn: () => Promise, - opts?: { maxRetries?: number; delay?: number; backoff?: number } -): Promise { - const maxRetries = opts?.maxRetries ?? 3 - const delay = opts?.delay ?? 0 - const backoff = opts?.backoff ?? 1 - - let retryCount = 0 - let latestDelay = delay - while (true) { - try { - return await fn() - } catch (err) { - retryCount++ - if (retryCount >= maxRetries) { - throw err - } - if (latestDelay > 0) { - await sleep(latestDelay) - latestDelay = latestDelay * backoff - } - } - } -} diff --git a/packages/core/src/shared/utilities/timeoutUtils.ts b/packages/core/src/shared/utilities/timeoutUtils.ts index 8dc38cfef5c..7c1a6e521ca 100644 --- a/packages/core/src/shared/utilities/timeoutUtils.ts +++ b/packages/core/src/shared/utilities/timeoutUtils.ts @@ -219,40 +219,101 @@ interface WaitUntilOptions { readonly interval?: number /** Wait for "truthy" result, else wait for any defined result including `false` (default: true) */ readonly truthy?: boolean + /** A backoff multiplier for how long the next interval will be (default: None, i.e 1) */ + readonly backoff?: number + /** + * Only retries when an error is thrown, otherwise returning the immediate result. + * - 'truthy' arg is ignored + * - If the timeout is reached it throws the last error + * - default: false + */ + readonly retryOnFail?: boolean } +export const waitUntilDefaultTimeout = 2000 +export const waitUntilDefaultInterval = 500 + /** - * Invokes `fn()` until it returns a truthy value (or non-undefined if `truthy:false`). + * Invokes `fn()` on an interval based on the given arguments. This can be used for retries, or until + * an expected result is given. Read {@link WaitUntilOptions} carefully. * * @param fn Function whose result is checked * @param options See {@link WaitUntilOptions} * - * @returns Result of `fn()`, or `undefined` if timeout was reached. + * @returns Result of `fn()`, or possibly `undefined` depending on the arguments. */ +export async function waitUntil(fn: () => Promise, options: WaitUntilOptions & { retryOnFail: true }): Promise +export async function waitUntil( + fn: () => Promise, + options: WaitUntilOptions & { retryOnFail: false } +): Promise +export async function waitUntil( + fn: () => Promise, + options: Omit +): Promise export async function waitUntil(fn: () => Promise, options: WaitUntilOptions): Promise { - const opt = { timeout: 5000, interval: 500, truthy: true, ...options } + // set default opts + const opt = { + timeout: waitUntilDefaultTimeout, + interval: waitUntilDefaultInterval, + truthy: true, + backoff: 1, + retryOnFail: false, + ...options, + } + + let interval = opt.interval + let lastError: Error | undefined + let elapsed: number = 0 + let remaining = opt.timeout + for (let i = 0; true; i++) { const start: number = globals.clock.Date.now() let result: T - // Needed in case a caller uses a 0 timeout (function is only called once) - if (opt.timeout > 0) { - result = await Promise.race([fn(), new Promise((r) => globals.clock.setTimeout(r, opt.timeout))]) - } else { - result = await fn() + try { + // Needed in case a caller uses a 0 timeout (function is only called once) + if (remaining > 0) { + result = await Promise.race([fn(), new Promise((r) => globals.clock.setTimeout(r, remaining))]) + } else { + result = await fn() + } + + if (opt.retryOnFail || (opt.truthy && result) || (!opt.truthy && result !== undefined)) { + return result + } + } catch (e) { + if (!opt.retryOnFail) { + throw e + } + + // Unlikely to hit this, but exists for typing + if (!(e instanceof Error)) { + throw e + } + + lastError = e } // Ensures that we never overrun the timeout - opt.timeout -= globals.clock.Date.now() - start + remaining -= globals.clock.Date.now() - start + + // If the sleep will exceed the timeout, abort early + if (elapsed + interval >= remaining) { + if (!opt.retryOnFail) { + return undefined + } - if ((opt.truthy && result) || (!opt.truthy && result !== undefined)) { - return result + throw lastError } - if (i * opt.interval >= opt.timeout) { - return undefined + + // when testing, this avoids the need to progress the stubbed clock + if (interval > 0) { + await sleep(interval) } - await sleep(opt.interval) + elapsed += interval + interval = interval * opt.backoff } } diff --git a/packages/core/src/test/notifications/controller.test.ts b/packages/core/src/test/notifications/controller.test.ts index e28f0271d19..761c6ce4bf2 100644 --- a/packages/core/src/test/notifications/controller.test.ts +++ b/packages/core/src/test/notifications/controller.test.ts @@ -6,7 +6,7 @@ import * as vscode from 'vscode' import * as FakeTimers from '@sinonjs/fake-timers' import assert from 'assert' -import sinon from 'sinon' +import sinon, { createSandbox } from 'sinon' import globals from '../../shared/extensionGlobals' import { randomUUID } from '../../shared/crypto' import { getContext } from '../../shared/vscode/setContext' @@ -27,6 +27,7 @@ import { import { HttpResourceFetcher } from '../../shared/resourcefetcher/httpResourceFetcher' import { NotificationsNode } from '../../notifications/panelNode' import { RuleEngine } from '../../notifications/rules' +import Sinon from 'sinon' // one test node to use across different tests export const panelNode: NotificationsNode = NotificationsNode.instance @@ -512,13 +513,16 @@ describe('Notifications Controller', function () { describe('RemoteFetcher', function () { let clock: FakeTimers.InstalledClock + let sandbox: Sinon.SinonSandbox before(function () { clock = installFakeClock() + sandbox = createSandbox() }) afterEach(function () { clock.reset() + sandbox.restore() }) after(function () { @@ -526,29 +530,29 @@ describe('RemoteFetcher', function () { }) it('retries and throws error', async function () { - const httpStub = sinon.stub(HttpResourceFetcher.prototype, 'getNewETagContent') + // Setup + const httpStub = sandbox.stub(HttpResourceFetcher.prototype, 'getNewETagContent') httpStub.throws(new Error('network error')) - const runClock = (async () => { - await clock.tickAsync(1) - for (let n = 1; n <= RemoteFetcher.retryNumber; n++) { - assert.equal(httpStub.callCount, n) - await clock.tickAsync(RemoteFetcher.retryIntervalMs) - } - - // Stop trying - await clock.tickAsync(RemoteFetcher.retryNumber) - assert.equal(httpStub.callCount, RemoteFetcher.retryNumber) - })() + // Start function under test + const fetcher = assert.rejects(new RemoteFetcher().fetch('startUp', 'any'), (e) => { + return e instanceof Error && e.message === 'last error' + }) - const fetcher = new RemoteFetcher() + // Progresses the clock, allowing the fetcher logic to break out of sleep for each iteration of withRetries() + assert.strictEqual(httpStub.callCount, 1) // 0 + await clock.tickAsync(RemoteFetcher.retryIntervalMs) + assert.strictEqual(httpStub.callCount, 2) // 30_000 + await clock.tickAsync(RemoteFetcher.retryIntervalMs) + assert.strictEqual(httpStub.callCount, 3) // 60_000 + await clock.tickAsync(RemoteFetcher.retryIntervalMs) + assert.strictEqual(httpStub.callCount, 4) // 120_000 + httpStub.throws(new Error('last error')) + await clock.tickAsync(RemoteFetcher.retryIntervalMs) + assert.strictEqual(httpStub.callCount, 5) // 150_000 + + // We hit timeout so the last error will be thrown await fetcher - .fetch('startUp', 'any') - .then(() => assert.ok(false, 'Did not throw exception.')) - .catch(() => assert.ok(true)) - await runClock - - httpStub.restore() }) }) diff --git a/packages/core/src/test/shared/utilities/functionUtils.test.ts b/packages/core/src/test/shared/utilities/functionUtils.test.ts index fb87cf3501e..43da4ebb619 100644 --- a/packages/core/src/test/shared/utilities/functionUtils.test.ts +++ b/packages/core/src/test/shared/utilities/functionUtils.test.ts @@ -4,10 +4,8 @@ */ import assert from 'assert' -import { once, onceChanged, debounce, withRetries } from '../../../shared/utilities/functionUtils' +import { once, onceChanged, debounce } from '../../../shared/utilities/functionUtils' import { installFakeClock } from '../../testUtil' -import { stub, SinonStub } from 'sinon' -import { InstalledClock } from '@sinonjs/fake-timers' describe('functionUtils', function () { it('once()', function () { @@ -109,82 +107,3 @@ describe('debounce', function () { }) }) }) - -// function to test the withRetries method. It passes in a stub function as the argument and has different tests that throw on different iterations -describe('withRetries', function () { - let clock: InstalledClock - let fn: SinonStub - - beforeEach(function () { - fn = stub() - clock = installFakeClock() - }) - - afterEach(function () { - clock.uninstall() - }) - - it('retries the function until it succeeds, using defaults', async function () { - fn.onCall(0).throws() - fn.onCall(1).throws() - fn.onCall(2).resolves('success') - assert.strictEqual(await withRetries(fn), 'success') - }) - - it('retries the function until it succeeds at the final try', async function () { - fn.onCall(0).throws() - fn.onCall(1).throws() - fn.onCall(2).throws() - fn.onCall(3).resolves('success') - assert.strictEqual(await withRetries(fn, { maxRetries: 4 }), 'success') - }) - - it('throws the last error if the function always fails, using defaults', async function () { - fn.onCall(0).throws() - fn.onCall(1).throws() - fn.onCall(2).throws() - fn.onCall(3).resolves('unreachable') - await assert.rejects(async () => { - await withRetries(fn) - }) - }) - - it('throws the last error if the function always fails', async function () { - fn.onCall(0).throws() - fn.onCall(1).throws() - fn.onCall(2).throws() - fn.onCall(3).throws() - fn.onCall(4).resolves('unreachable') - await assert.rejects(async () => { - await withRetries(fn, { maxRetries: 4 }) - }) - }) - - it('honors retry delay + backoff multiplier', async function () { - fn.onCall(0).throws() // 100ms - fn.onCall(1).throws() // 200ms - fn.onCall(2).throws() // 400ms - fn.onCall(3).resolves('success') - - const res = withRetries(fn, { maxRetries: 4, delay: 100, backoff: 2 }) - - // Check the call count after each iteration, ensuring the function is called - // after the correct delay between retries. - await clock.tickAsync(99) - assert.strictEqual(fn.callCount, 1) - await clock.tickAsync(1) - assert.strictEqual(fn.callCount, 2) - - await clock.tickAsync(199) - assert.strictEqual(fn.callCount, 2) - await clock.tickAsync(1) - assert.strictEqual(fn.callCount, 3) - - await clock.tickAsync(399) - assert.strictEqual(fn.callCount, 3) - await clock.tickAsync(1) - assert.strictEqual(fn.callCount, 4) - - assert.strictEqual(await res, 'success') - }) -}) diff --git a/packages/core/src/test/shared/utilities/timeoutUtils.test.ts b/packages/core/src/test/shared/utilities/timeoutUtils.test.ts index c518b1cfae1..3ee5968132b 100644 --- a/packages/core/src/test/shared/utilities/timeoutUtils.test.ts +++ b/packages/core/src/test/shared/utilities/timeoutUtils.test.ts @@ -7,7 +7,7 @@ import assert from 'assert' import * as FakeTimers from '@sinonjs/fake-timers' import * as timeoutUtils from '../../../shared/utilities/timeoutUtils' import { installFakeClock, tickPromise } from '../../../test/testUtil' -import { sleep } from '../../../shared/utilities/timeoutUtils' +import { sleep, waitUntil } from '../../../shared/utilities/timeoutUtils' import { SinonStub, SinonSandbox, createSandbox } from 'sinon' // We export this describe() so it can be used in the web tests as well @@ -303,6 +303,17 @@ export const timeoutUtilsDescribe = describe('timeoutUtils', async function () { assert.strictEqual(returnValue, testSettings.callGoal) }) + it('returns value after multiple function calls WITH backoff', async function () { + testSettings.callGoal = 4 + const returnValue: number | undefined = await timeoutUtils.waitUntil(testFunction, { + timeout: 10000, + interval: 10, + truthy: false, + backoff: 2, + }) + assert.strictEqual(returnValue, testSettings.callGoal) + }) + it('timeout before function returns defined value', async function () { testSettings.callGoal = 7 const returnValue: number | undefined = await timeoutUtils.waitUntil(testFunction, { @@ -376,6 +387,90 @@ export const timeoutUtilsDescribe = describe('timeoutUtils', async function () { }) }) + describe('waitUntil w/ retries', function () { + let fn: SinonStub<[], Promise> + + beforeEach(function () { + fn = sandbox.stub() + }) + + it('retries the function until it succeeds', async function () { + fn.onCall(0).throws() + fn.onCall(1).throws() + fn.onCall(2).resolves('success') + + const res = waitUntil(fn, { retryOnFail: true }) + + 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('retryOnFail ignores truthiness', async function () { + fn.resolves(false) + const res = waitUntil(fn, { retryOnFail: true, truthy: true }) + assert.strictEqual(await res, false) + }) + + // This test causes the following error, cannot figure out why: + // `rejected promise not handled within 1 second: Error: last` + it('throws the last error if the function always fails, using defaults', async function () { + fn.onCall(0).throws() // 0 + fn.onCall(1).throws() // 500 + fn.onCall(2).throws(new Error('second last')) // 1000 + fn.onCall(3).throws(new Error('last')) // 1500 + fn.onCall(4).resolves('this is not hit') + + // We must wrap w/ assert.rejects() here instead of at the end, otherwise Mocha raise a + // `rejected promise not handled within 1 second: Error: last` + const res = assert.rejects( + waitUntil(fn, { retryOnFail: true }), + (e) => e instanceof Error && e.message === 'last' + ) + + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) // 500 + assert.strictEqual(fn.callCount, 2) + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) // 1000 + assert.strictEqual(fn.callCount, 3) + await clock.tickAsync(timeoutUtils.waitUntilDefaultInterval) // 1500 + assert.strictEqual(fn.callCount, 4) + + await res + }) + + it('honors retry delay + backoff multiplier', async function () { + fn.onCall(0).throws(Error('0')) // 0ms + fn.onCall(1).throws(Error('1')) // 100ms + fn.onCall(2).throws(Error('2')) // 200ms + fn.onCall(3).resolves('success') // 400ms + + // Note 701 instead of 700 for timeout. The 1 millisecond allows the final call to execute + // since the timeout condition is >= instead of > + const res = waitUntil(fn, { timeout: 701, interval: 100, backoff: 2, retryOnFail: true }) + + // Check the call count after each iteration, ensuring the function is called + // after the correct delay between retries. + await clock.tickAsync(99) + assert.strictEqual(fn.callCount, 1) + await clock.tickAsync(1) + assert.strictEqual(fn.callCount, 2) + + await clock.tickAsync(199) + assert.strictEqual(fn.callCount, 2) + await clock.tickAsync(1) + assert.strictEqual(fn.callCount, 3) + + await clock.tickAsync(399) + assert.strictEqual(fn.callCount, 3) + await clock.tickAsync(1) + assert.strictEqual(fn.callCount, 4) + + assert.strictEqual(await res, 'success') + }) + }) + describe('waitTimeout', async function () { async function testFunction(delay: number = 500, error?: Error) { await sleep(delay)