diff --git a/packages/controller-utils/CHANGELOG.md b/packages/controller-utils/CHANGELOG.md index 487e484d820..92c606edc1b 100644 --- a/packages/controller-utils/CHANGELOG.md +++ b/packages/controller-utils/CHANGELOG.md @@ -36,6 +36,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `handleWhen` - These symbols will be removed in a future major version. Please use equivalent implementations from `@metamask/base-data-service` going forward. +### Fixed + +- Fix `fetchWithErrorHandling` ignoring its `timeout` option ([#10048](https://github.com/MetaMask/core/pull/10048)) + - Previously, the fetch was awaited before `Promise.race` was constructed, so the race always resolved to the already-settled fetch and the timeout could never fire. + ## [12.3.0] ### Added diff --git a/packages/controller-utils/src/util.test.ts b/packages/controller-utils/src/util.test.ts index e3346ea473f..d029c63e350 100644 --- a/packages/controller-utils/src/util.test.ts +++ b/packages/controller-utils/src/util.test.ts @@ -561,6 +561,47 @@ describe('util', () => { }); }); + describe('fetchWithErrorHandling', () => { + it('should fetch first if response is faster than timeout', async () => { + nock(SOME_API).get(/.+/u).delay(50).reply(200, { foo: 'bar' }); + const result = await util.fetchWithErrorHandling({ + url: SOME_API, + timeout: 300, + }); + expect(result).toStrictEqual({ foo: 'bar' }); + }); + + it('should stop waiting once the timeout elapses, even if the fetch never resolves in time', async () => { + nock(SOME_API).get(/.+/u).delay(300).reply(200, { foo: 'bar' }); + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(); + const start = Date.now(); + const result = await util.fetchWithErrorHandling({ + url: SOME_API, + timeout: 50, + }); + const elapsed = Date.now() - start; + // The timeout error is logged (not thrown) and the result is + // undefined, matching how a caught+swallowed error already behaves. + expect(result).toBeUndefined(); + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.objectContaining({ message: 'timeout' }), + ); + // If the timeout is honored, this resolves in ~50ms, not ~300ms. + // On unfixed code, the outer `await` inside the race already + // resolves the fetch before the race is constructed, so this takes + // the full 300ms instead. The bound is loose (well under the 300ms + // delay, generous above the 50ms timeout) to avoid CI flakiness on a + // loaded worker; the `result` assertion above is the primary proof. + expect(elapsed).toBeLessThan(250); + consoleErrorSpy.mockRestore(); + // Let the abandoned nock interceptor settle before the test ends, + // so it doesn't leave a dangling timer past the process's teardown. + await new Promise((resolve) => setTimeout(resolve, 300)); + }); + }); + describe('normalizeEnsName', () => { it('should normalize with valid 2LD', async () => { let valid = util.normalizeEnsName('metamask.eth'); diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index 71a0f8c5430..6472fb7123a 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -486,9 +486,9 @@ export async function fetchWithErrorHandling({ let result; try { if (timeout) { - result = Promise.race([ - await handleFetch(url, options), - new Promise((_resolve, reject) => + result = await Promise.race([ + handleFetch(url, options), + new Promise((_resolve, reject) => setTimeout(() => { reject(TIMEOUT_ERROR); }, timeout),