From db4725be5d51d616705eb0b3d7beb6e77b83ac7f Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 13 Nov 2024 14:09:06 +0100 Subject: [PATCH 01/25] fix: internal listeners infinite retry loop --- src/PollingBlockTracker.ts | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index 8ab89b99..137ba667 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -10,8 +10,6 @@ const log = createModuleLogger(projectLogger, 'polling-block-tracker'); const createRandomId = getCreateRandomId(); const sec = 1000; -const calculateSum = (accumulator: number, currentValue: number) => - accumulator + currentValue; const blockTrackerEvents: (string | symbol)[] = ['sync', 'latest']; export interface PollingBlockTrackerOptions { @@ -54,6 +52,10 @@ export class PollingBlockTracker private readonly _setSkipCacheFlag: boolean; + readonly #onLatestBlockInternalListeners: (( + value: string | PromiseLike, + ) => void)[] = []; + constructor(opts: PollingBlockTrackerOptions = {}) { // parse + validate args if (!opts.provider) { @@ -106,9 +108,17 @@ export class PollingBlockTracker return this._currentBlock; } // wait for a new latest block - const latestBlock: string = await new Promise((resolve) => - this.once('latest', resolve), - ); + const latestBlock: string = await new Promise((resolve) => { + const listener = (value: string | PromiseLike) => { + this.#onLatestBlockInternalListeners.splice( + this.#onLatestBlockInternalListeners.indexOf(listener), + 1, + ); + resolve(value); + }; + this.#onLatestBlockInternalListeners.push(listener); + this.once('latest', listener); + }); // return newly set current block return latestBlock; } @@ -180,8 +190,13 @@ export class PollingBlockTracker private _getBlockTrackerEventCount(): number { return blockTrackerEvents - .map((eventName) => this.listenerCount(eventName)) - .reduce(calculateSum); + .map((eventName) => this.listeners(eventName)) + .flat() + .filter((listener) => + this.#onLatestBlockInternalListeners.every( + (internalListener) => !Object.is(internalListener, listener), + ), + ).length; } private _shouldUseNewBlock(newBlock: string) { From 4fbcd76ede2cbe33bb889249e66f22e9adb5dd2e Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 13 Nov 2024 14:20:11 +0100 Subject: [PATCH 02/25] edit `CHANGELOG` --- CHANGELOG.md | 2 ++ src/PollingBlockTracker.ts | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 366dbe4c..19101de5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed +- Avoid risk of infinte retry loops when fetching new blocks ([#284](https://github.com/MetaMask/eth-block-tracker/pull/284)) ## [11.0.2] ### Fixed diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index 137ba667..e0d872ee 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -189,14 +189,17 @@ export class PollingBlockTracker } private _getBlockTrackerEventCount(): number { - return blockTrackerEvents - .map((eventName) => this.listeners(eventName)) - .flat() - .filter((listener) => - this.#onLatestBlockInternalListeners.every( - (internalListener) => !Object.is(internalListener, listener), - ), - ).length; + return ( + blockTrackerEvents + .map((eventName) => this.listeners(eventName)) + .flat() + // internal listeners are not included in the count + .filter((listener) => + this.#onLatestBlockInternalListeners.every( + (internalListener) => !Object.is(internalListener, listener), + ), + ).length + ); } private _shouldUseNewBlock(newBlock: string) { From ec76fae3572d1364dbd345f561d089f73e5c423e Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 13 Nov 2024 15:03:34 +0100 Subject: [PATCH 03/25] refactor: rename to `#internalEventListeners` --- src/PollingBlockTracker.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index e0d872ee..2a4bdc50 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -52,7 +52,7 @@ export class PollingBlockTracker private readonly _setSkipCacheFlag: boolean; - readonly #onLatestBlockInternalListeners: (( + readonly #internalEventListeners: (( value: string | PromiseLike, ) => void)[] = []; @@ -110,13 +110,13 @@ export class PollingBlockTracker // wait for a new latest block const latestBlock: string = await new Promise((resolve) => { const listener = (value: string | PromiseLike) => { - this.#onLatestBlockInternalListeners.splice( - this.#onLatestBlockInternalListeners.indexOf(listener), + this.#internalEventListeners.splice( + this.#internalEventListeners.indexOf(listener), 1, ); resolve(value); }; - this.#onLatestBlockInternalListeners.push(listener); + this.#internalEventListeners.push(listener); this.once('latest', listener); }); // return newly set current block @@ -195,7 +195,7 @@ export class PollingBlockTracker .flat() // internal listeners are not included in the count .filter((listener) => - this.#onLatestBlockInternalListeners.every( + this.#internalEventListeners.every( (internalListener) => !Object.is(internalListener, listener), ), ).length From eb5514d510af7086873adf951962f4882d763747 Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Wed, 13 Nov 2024 16:43:22 +0100 Subject: [PATCH 04/25] refactor: simplify listener equality check Co-authored-by: Mark Stacey --- src/PollingBlockTracker.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index 2a4bdc50..80011494 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -195,9 +195,7 @@ export class PollingBlockTracker .flat() // internal listeners are not included in the count .filter((listener) => - this.#internalEventListeners.every( - (internalListener) => !Object.is(internalListener, listener), - ), + this.#internalEventListeners.includes(listener), ).length ); } From a6c6e3c0af8fe54f9a9895032180354c708f183c Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 14 Nov 2024 11:22:40 +0100 Subject: [PATCH 05/25] Revert "refactor: simplify listener equality check" This reverts commit eb5514d510af7086873adf951962f4882d763747. --- src/PollingBlockTracker.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index 80011494..2a4bdc50 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -195,7 +195,9 @@ export class PollingBlockTracker .flat() // internal listeners are not included in the count .filter((listener) => - this.#internalEventListeners.includes(listener), + this.#internalEventListeners.every( + (internalListener) => !Object.is(internalListener, listener), + ), ).length ); } From b724fc96b8f1db51fd09af2b7915a8c982fd9738 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 14 Nov 2024 11:29:20 +0100 Subject: [PATCH 06/25] apply fix to `SubscribeBlockTracker` --- src/SubscribeBlockTracker.ts | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/SubscribeBlockTracker.ts b/src/SubscribeBlockTracker.ts index d81a8ea9..eabe35cc 100644 --- a/src/SubscribeBlockTracker.ts +++ b/src/SubscribeBlockTracker.ts @@ -9,8 +9,6 @@ const createRandomId = getCreateRandomId(); const sec = 1000; -const calculateSum = (accumulator: number, currentValue: number) => - accumulator + currentValue; const blockTrackerEvents: (string | symbol)[] = ['sync', 'latest']; export interface SubscribeBlockTrackerOptions { @@ -43,6 +41,10 @@ export class SubscribeBlockTracker private _subscriptionId: string | null; + readonly #internalEventListeners: (( + value: string | PromiseLike, + ) => void)[] = []; + constructor(opts: SubscribeBlockTrackerOptions = {}) { // parse + validate args if (!opts.provider) { @@ -91,9 +93,17 @@ export class SubscribeBlockTracker return this._currentBlock; } // wait for a new latest block - const latestBlock: string = await new Promise((resolve) => - this.once('latest', resolve), - ); + const latestBlock: string = await new Promise((resolve) => { + const listener = (value: string | PromiseLike) => { + this.#internalEventListeners.splice( + this.#internalEventListeners.indexOf(listener), + 1, + ); + resolve(value); + }; + this.#internalEventListeners.push(listener); + this.once('latest', listener); + }); // return newly set current block return latestBlock; } @@ -162,9 +172,17 @@ export class SubscribeBlockTracker } private _getBlockTrackerEventCount(): number { - return blockTrackerEvents - .map((eventName) => this.listenerCount(eventName)) - .reduce(calculateSum); + return ( + blockTrackerEvents + .map((eventName) => this.listeners(eventName)) + .flat() + // internal listeners are not included in the count + .filter((listener) => + this.#internalEventListeners.every( + (internalListener) => !Object.is(internalListener, listener), + ), + ).length + ); } private _shouldUseNewBlock(newBlock: string) { From 3f39061b910606419ebe0b969fabfebe2c3a6b52 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 20 Nov 2024 15:22:39 +0100 Subject: [PATCH 07/25] fix: reject `getLatestBlock` promise when block tracker stops --- src/PollingBlockTracker.test.ts | 27 ++++++++++++ src/PollingBlockTracker.ts | 71 +++++++++++++++++++++++++------ src/SubscribeBlockTracker.test.ts | 27 ++++++++++++ src/SubscribeBlockTracker.ts | 58 +++++++++++++++++++++---- 4 files changed, 162 insertions(+), 21 deletions(-) diff --git a/src/PollingBlockTracker.test.ts b/src/PollingBlockTracker.test.ts index 7a598db2..c74dc137 100644 --- a/src/PollingBlockTracker.test.ts +++ b/src/PollingBlockTracker.test.ts @@ -185,6 +185,33 @@ describe('PollingBlockTracker', () => { ); }); + it('should not retry failed requests after the block tracker is stopped', async () => { + recordCallsToSetTimeout({ numAutomaticCalls: 1 }); + + await withPollingBlockTracker( + { + provider: { + stubs: [ + { + methodName: 'eth_blockNumber', + error: 'boom', + }, + ], + }, + }, + async ({ blockTracker }) => { + const latestBlockPromise = blockTracker.getLatestBlock(); + + expect(blockTracker.isRunning()).toBe(true); + await blockTracker.destroy(); + await expect(latestBlockPromise).rejects.toThrow( + 'Block tracker ended before latest block was available', + ); + expect(blockTracker.isRunning()).toBe(false); + }, + ); + }); + it('request the latest block number with `skipCache: true` if the block tracker was initialized with `setSkipCacheFlag: true`', async () => { recordCallsToSetTimeout(); diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index 2a4bdc50..2fc334dd 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -26,6 +26,8 @@ interface ExtendedJsonRpcRequest extends JsonRpcRequest<[]> { skipCache?: boolean; } +type InternalListener = (value: string | PromiseLike) => void; + export class PollingBlockTracker extends SafeEventEmitter implements BlockTracker @@ -52,9 +54,7 @@ export class PollingBlockTracker private readonly _setSkipCacheFlag: boolean; - readonly #internalEventListeners: (( - value: string | PromiseLike, - ) => void)[] = []; + readonly #internalEventListeners: InternalListener[] = []; constructor(opts: PollingBlockTrackerOptions = {}) { // parse + validate args @@ -91,7 +91,19 @@ export class PollingBlockTracker async destroy() { this._cancelBlockResetTimeout(); this._maybeEnd(); - super.removeAllListeners(); + this.eventNames().forEach((eventName) => + this.listeners(eventName).forEach((listener) => { + if ( + this.#internalEventListeners.every( + (internalListener) => !Object.is(internalListener, listener), + ) + ) { + // @ts-expect-error this listener comes from SafeEventEmitter itself, though + // its type differs between `.listeners()` and `.removeListener()` + this.removeListener(eventName, listener); + } + }), + ); } isRunning(): boolean { @@ -108,16 +120,31 @@ export class PollingBlockTracker return this._currentBlock; } // wait for a new latest block - const latestBlock: string = await new Promise((resolve) => { - const listener = (value: string | PromiseLike) => { - this.#internalEventListeners.splice( - this.#internalEventListeners.indexOf(listener), - 1, - ); + const latestBlock: string = await new Promise((resolve, reject) => { + // eslint-disable-next-line prefer-const + let onLatestBlockUnavailable: InternalListener; + const onLatestBlockAvailable = (value: string | PromiseLike) => { + this.#removeInternalListener(onLatestBlockAvailable); + this.removeListener('error', onLatestBlockUnavailable); resolve(value); }; - this.#internalEventListeners.push(listener); - this.once('latest', listener); + onLatestBlockUnavailable = () => { + // if the block tracker is no longer running, reject + // and remove the listeners + if (!this._isRunning) { + this.#removeInternalListener(onLatestBlockAvailable); + this.#removeInternalListener(onLatestBlockUnavailable); + this.removeListener('latest', onLatestBlockAvailable); + this.removeListener('error', onLatestBlockUnavailable); + reject( + new Error('Block tracker ended before latest block was available'), + ); + } + }; + this.#addInternalListener(onLatestBlockAvailable); + this.#addInternalListener(onLatestBlockUnavailable); + this.once('latest', onLatestBlockAvailable); + this.on('error', onLatestBlockUnavailable); }); // return newly set current block return latestBlock; @@ -317,6 +344,15 @@ export class PollingBlockTracker try { this.emit('error', newErr); + if ( + this.listeners('error').filter((listener) => + this.#internalEventListeners.every( + (internalListener) => !Object.is(listener, internalListener), + ), + ).length === 0 + ) { + console.error(newErr); + } } catch (emitErr) { console.error(newErr); } @@ -351,6 +387,17 @@ export class PollingBlockTracker this._pollingTimeout = undefined; } } + + #addInternalListener(listener: InternalListener) { + this.#internalEventListeners.push(listener); + } + + #removeInternalListener(listener: InternalListener) { + this.#internalEventListeners.splice( + this.#internalEventListeners.indexOf(listener), + 1, + ); + } } /** diff --git a/src/SubscribeBlockTracker.test.ts b/src/SubscribeBlockTracker.test.ts index 6d78f230..bb99ce34 100644 --- a/src/SubscribeBlockTracker.test.ts +++ b/src/SubscribeBlockTracker.test.ts @@ -151,6 +151,33 @@ describe('SubscribeBlockTracker', () => { }); }); + it('should not retry failed requests after the block tracker is stopped', async () => { + recordCallsToSetTimeout({ numAutomaticCalls: 1 }); + + await withSubscribeBlockTracker( + { + provider: { + stubs: [ + { + methodName: 'eth_blockNumber', + error: 'boom', + }, + ], + }, + }, + async ({ blockTracker }) => { + const latestBlockPromise = blockTracker[methodToGetLatestBlock](); + + expect(blockTracker.isRunning()).toBe(true); + await blockTracker.destroy(); + await expect(latestBlockPromise).rejects.toThrow( + 'Block tracker ended before latest block was available', + ); + expect(blockTracker.isRunning()).toBe(false); + }, + ); + }); + it('should fetch the latest block number', async () => { recordCallsToSetTimeout(); diff --git a/src/SubscribeBlockTracker.ts b/src/SubscribeBlockTracker.ts index eabe35cc..18e0074c 100644 --- a/src/SubscribeBlockTracker.ts +++ b/src/SubscribeBlockTracker.ts @@ -23,6 +23,8 @@ interface SubscriptionNotificationParams { result: { number: string }; } +type InternalListener = (value: string | PromiseLike) => void; + export class SubscribeBlockTracker extends SafeEventEmitter implements BlockTracker @@ -76,7 +78,19 @@ export class SubscribeBlockTracker async destroy() { this._cancelBlockResetTimeout(); await this._maybeEnd(); - super.removeAllListeners(); + this.eventNames().forEach((eventName) => + this.listeners(eventName).forEach((listener) => { + if ( + this.#internalEventListeners.every( + (internalListener) => !Object.is(internalListener, listener), + ) + ) { + // @ts-expect-error this listener comes from SafeEventEmitter itself, though + // its type differs between `.listeners()` and `.removeListener()` + this.removeListener(eventName, listener); + } + }), + ); } isRunning(): boolean { @@ -93,16 +107,31 @@ export class SubscribeBlockTracker return this._currentBlock; } // wait for a new latest block - const latestBlock: string = await new Promise((resolve) => { - const listener = (value: string | PromiseLike) => { - this.#internalEventListeners.splice( - this.#internalEventListeners.indexOf(listener), - 1, - ); + const latestBlock: string = await new Promise((resolve, reject) => { + // eslint-disable-next-line prefer-const + let onLatestBlockUnavailable: InternalListener; + const onLatestBlockAvailable = (value: string | PromiseLike) => { + this.#removeInternalListener(onLatestBlockAvailable); + this.removeListener('error', onLatestBlockUnavailable); resolve(value); }; - this.#internalEventListeners.push(listener); - this.once('latest', listener); + onLatestBlockUnavailable = () => { + // if the block tracker is no longer running, reject + // and remove the listeners + if (!this._isRunning) { + this.#removeInternalListener(onLatestBlockAvailable); + this.#removeInternalListener(onLatestBlockUnavailable); + this.removeListener('latest', onLatestBlockAvailable); + this.removeListener('error', onLatestBlockUnavailable); + reject( + new Error('Block tracker ended before latest block was available'), + ); + } + }; + this.#addInternalListener(onLatestBlockAvailable); + this.#addInternalListener(onLatestBlockUnavailable); + this.once('latest', onLatestBlockAvailable); + this.on('error', onLatestBlockUnavailable); }); // return newly set current block return latestBlock; @@ -289,6 +318,17 @@ export class SubscribeBlockTracker this._newPotentialLatest(response.params.result.number); } } + + #addInternalListener(listener: InternalListener) { + this.#internalEventListeners.push(listener); + } + + #removeInternalListener(listener: InternalListener) { + this.#internalEventListeners.splice( + this.#internalEventListeners.indexOf(listener), + 1, + ); + } } /** From 16f88ff289407cc8faa1ba190a437e25c7de8d84 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 20 Nov 2024 15:34:28 +0100 Subject: [PATCH 08/25] fix: remove on error internal listener --- src/PollingBlockTracker.ts | 1 + src/SubscribeBlockTracker.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index 2fc334dd..d5613190 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -125,6 +125,7 @@ export class PollingBlockTracker let onLatestBlockUnavailable: InternalListener; const onLatestBlockAvailable = (value: string | PromiseLike) => { this.#removeInternalListener(onLatestBlockAvailable); + this.#removeInternalListener(onLatestBlockUnavailable); this.removeListener('error', onLatestBlockUnavailable); resolve(value); }; diff --git a/src/SubscribeBlockTracker.ts b/src/SubscribeBlockTracker.ts index 18e0074c..f83fd3cb 100644 --- a/src/SubscribeBlockTracker.ts +++ b/src/SubscribeBlockTracker.ts @@ -112,6 +112,7 @@ export class SubscribeBlockTracker let onLatestBlockUnavailable: InternalListener; const onLatestBlockAvailable = (value: string | PromiseLike) => { this.#removeInternalListener(onLatestBlockAvailable); + this.#removeInternalListener(onLatestBlockUnavailable); this.removeListener('error', onLatestBlockUnavailable); resolve(value); }; From e77ed5db977256ddb84bdf32375868d23aac2c4c Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 26 Nov 2024 11:55:36 -0330 Subject: [PATCH 09/25] Suggestion to simplify prevention of dangling Promise on destroy (#286) * Suggestion to simplify prevention of dangling Promise on destroy The `fix/internal-listeners` branch has a number of changes intended to ensure we don't have a dangling unresolved Promise when the block tracker is destroyed. This solution involved adding an additional listener to capture errors, and it involved not removing internal listeners when `destroy` is called. This required changes to some logging in `_updateAndQueue` as well. This commit is an alternative solution that avoids the use of internal listeners, thus avoiding much of the complexity in the previous solution. Instead an internal deferred Promise is used. This also might be slightly more efficient when `getLatestBlock` is called repeatedly, as we can reuse the same listener rather than creating a new one each time. * Unset pending latest block after it has resolved --- src/PollingBlockTracker.ts | 83 ++++++++++++++------------------------ 1 file changed, 30 insertions(+), 53 deletions(-) diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index d5613190..a59d540e 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -1,6 +1,10 @@ import type { SafeEventEmitterProvider } from '@metamask/eth-json-rpc-provider'; import SafeEventEmitter from '@metamask/safe-event-emitter'; -import { getErrorMessage, type JsonRpcRequest } from '@metamask/utils'; +import { + createDeferredPromise, + getErrorMessage, + type JsonRpcRequest, +} from '@metamask/utils'; import getCreateRandomId from 'json-rpc-random-id'; import type { BlockTracker } from './BlockTracker'; @@ -26,7 +30,7 @@ interface ExtendedJsonRpcRequest extends JsonRpcRequest<[]> { skipCache?: boolean; } -type InternalListener = (value: string | PromiseLike) => void; +type InternalListener = (value: string) => void; export class PollingBlockTracker extends SafeEventEmitter @@ -56,6 +60,10 @@ export class PollingBlockTracker readonly #internalEventListeners: InternalListener[] = []; + #pendingLatestBlock: + | { promise: Promise; reject: (error: unknown) => void } + | undefined; + constructor(opts: PollingBlockTrackerOptions = {}) { // parse + validate args if (!opts.provider) { @@ -91,19 +99,10 @@ export class PollingBlockTracker async destroy() { this._cancelBlockResetTimeout(); this._maybeEnd(); - this.eventNames().forEach((eventName) => - this.listeners(eventName).forEach((listener) => { - if ( - this.#internalEventListeners.every( - (internalListener) => !Object.is(internalListener, listener), - ) - ) { - // @ts-expect-error this listener comes from SafeEventEmitter itself, though - // its type differs between `.listeners()` and `.removeListener()` - this.removeListener(eventName, listener); - } - }), - ); + this.removeAllListeners(); + if (this.#pendingLatestBlock) { + this.#pendingLatestBlock.reject(new Error('Block tracker destroeyd')); + } } isRunning(): boolean { @@ -118,37 +117,24 @@ export class PollingBlockTracker // return if available if (this._currentBlock) { return this._currentBlock; + } else if (this.#pendingLatestBlock) { + return await this.#pendingLatestBlock.promise; } - // wait for a new latest block - const latestBlock: string = await new Promise((resolve, reject) => { - // eslint-disable-next-line prefer-const - let onLatestBlockUnavailable: InternalListener; - const onLatestBlockAvailable = (value: string | PromiseLike) => { - this.#removeInternalListener(onLatestBlockAvailable); - this.#removeInternalListener(onLatestBlockUnavailable); - this.removeListener('error', onLatestBlockUnavailable); - resolve(value); - }; - onLatestBlockUnavailable = () => { - // if the block tracker is no longer running, reject - // and remove the listeners - if (!this._isRunning) { - this.#removeInternalListener(onLatestBlockAvailable); - this.#removeInternalListener(onLatestBlockUnavailable); - this.removeListener('latest', onLatestBlockAvailable); - this.removeListener('error', onLatestBlockUnavailable); - reject( - new Error('Block tracker ended before latest block was available'), - ); - } - }; - this.#addInternalListener(onLatestBlockAvailable); - this.#addInternalListener(onLatestBlockUnavailable); - this.once('latest', onLatestBlockAvailable); - this.on('error', onLatestBlockUnavailable); + + const { promise, resolve, reject } = createDeferredPromise({ + suppressUnhandledRejection: true, }); - // return newly set current block - return latestBlock; + this.#pendingLatestBlock = { promise, reject }; + + // wait for a new latest block + const onLatestBlock = (value: string) => { + this.#removeInternalListener(onLatestBlock); + resolve(value); + delete this.#pendingLatestBlock; + }; + this.#addInternalListener(onLatestBlock); + this.once('latest', onLatestBlock); + return await promise; } // dont allow module consumer to remove our internal event listeners @@ -345,15 +331,6 @@ export class PollingBlockTracker try { this.emit('error', newErr); - if ( - this.listeners('error').filter((listener) => - this.#internalEventListeners.every( - (internalListener) => !Object.is(listener, internalListener), - ), - ).length === 0 - ) { - console.error(newErr); - } } catch (emitErr) { console.error(newErr); } From e89e0585fc039d7678e269da07a1b885b5793a6c Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Tue, 26 Nov 2024 16:31:07 +0100 Subject: [PATCH 10/25] fix: operand of a delete operator cannot be a private identifier --- src/PollingBlockTracker.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index a59d540e..d17a10b4 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -130,7 +130,7 @@ export class PollingBlockTracker const onLatestBlock = (value: string) => { this.#removeInternalListener(onLatestBlock); resolve(value); - delete this.#pendingLatestBlock; + this.#pendingLatestBlock = undefined; }; this.#addInternalListener(onLatestBlock); this.once('latest', onLatestBlock); From cbcb2c637b22151d4bf77b57b48a119836776d1a Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 27 Nov 2024 12:09:34 +0100 Subject: [PATCH 11/25] test: change error message --- src/PollingBlockTracker.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PollingBlockTracker.test.ts b/src/PollingBlockTracker.test.ts index c74dc137..15f18f8d 100644 --- a/src/PollingBlockTracker.test.ts +++ b/src/PollingBlockTracker.test.ts @@ -205,7 +205,7 @@ describe('PollingBlockTracker', () => { expect(blockTracker.isRunning()).toBe(true); await blockTracker.destroy(); await expect(latestBlockPromise).rejects.toThrow( - 'Block tracker ended before latest block was available', + 'Block tracker destroeyd', ); expect(blockTracker.isRunning()).toBe(false); }, From a568006f9257315d05fd64311489e8669de52f70 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 27 Nov 2024 13:18:37 +0100 Subject: [PATCH 12/25] refactor: add helper methods --- src/PollingBlockTracker.test.ts | 2 +- src/PollingBlockTracker.ts | 26 ++++++++++++++++---------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/PollingBlockTracker.test.ts b/src/PollingBlockTracker.test.ts index 15f18f8d..2511dd34 100644 --- a/src/PollingBlockTracker.test.ts +++ b/src/PollingBlockTracker.test.ts @@ -205,7 +205,7 @@ describe('PollingBlockTracker', () => { expect(blockTracker.isRunning()).toBe(true); await blockTracker.destroy(); await expect(latestBlockPromise).rejects.toThrow( - 'Block tracker destroeyd', + 'Block tracker destroyed', ); expect(blockTracker.isRunning()).toBe(false); }, diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index d17a10b4..815d0806 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -2,6 +2,7 @@ import type { SafeEventEmitterProvider } from '@metamask/eth-json-rpc-provider'; import SafeEventEmitter from '@metamask/safe-event-emitter'; import { createDeferredPromise, + type DeferredPromise, getErrorMessage, type JsonRpcRequest, } from '@metamask/utils'; @@ -60,9 +61,7 @@ export class PollingBlockTracker readonly #internalEventListeners: InternalListener[] = []; - #pendingLatestBlock: - | { promise: Promise; reject: (error: unknown) => void } - | undefined; + #pendingLatestBlock?: DeferredPromise; constructor(opts: PollingBlockTrackerOptions = {}) { // parse + validate args @@ -99,10 +98,8 @@ export class PollingBlockTracker async destroy() { this._cancelBlockResetTimeout(); this._maybeEnd(); - this.removeAllListeners(); - if (this.#pendingLatestBlock) { - this.#pendingLatestBlock.reject(new Error('Block tracker destroeyd')); - } + super.removeAllListeners(); + this.#rejectPendingLatestBlock(new Error('Block tracker destroyed')); } isRunning(): boolean { @@ -124,13 +121,12 @@ export class PollingBlockTracker const { promise, resolve, reject } = createDeferredPromise({ suppressUnhandledRejection: true, }); - this.#pendingLatestBlock = { promise, reject }; + this.#pendingLatestBlock = { promise, resolve, reject }; // wait for a new latest block const onLatestBlock = (value: string) => { this.#removeInternalListener(onLatestBlock); - resolve(value); - this.#pendingLatestBlock = undefined; + this.#resolvePendingLatestBlock(value); }; this.#addInternalListener(onLatestBlock); this.once('latest', onLatestBlock); @@ -376,6 +372,16 @@ export class PollingBlockTracker 1, ); } + + #resolvePendingLatestBlock(value: string) { + this.#pendingLatestBlock?.resolve(value); + this.#pendingLatestBlock = undefined; + } + + #rejectPendingLatestBlock(error: unknown) { + this.#pendingLatestBlock?.reject(error); + this.#pendingLatestBlock = undefined; + } } /** From 416f51a99e4bb95065357ec5b4ac436a88f4e573 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 27 Nov 2024 13:19:37 +0100 Subject: [PATCH 13/25] refactor: simplify `SubscribeBlockTracker` --- src/SubscribeBlockTracker.test.ts | 2 +- src/SubscribeBlockTracker.ts | 84 ++++++++++++++----------------- 2 files changed, 38 insertions(+), 48 deletions(-) diff --git a/src/SubscribeBlockTracker.test.ts b/src/SubscribeBlockTracker.test.ts index bb99ce34..f8d8dd8b 100644 --- a/src/SubscribeBlockTracker.test.ts +++ b/src/SubscribeBlockTracker.test.ts @@ -171,7 +171,7 @@ describe('SubscribeBlockTracker', () => { expect(blockTracker.isRunning()).toBe(true); await blockTracker.destroy(); await expect(latestBlockPromise).rejects.toThrow( - 'Block tracker ended before latest block was available', + 'Block tracker destroyed', ); expect(blockTracker.isRunning()).toBe(false); }, diff --git a/src/SubscribeBlockTracker.ts b/src/SubscribeBlockTracker.ts index f83fd3cb..386ea18f 100644 --- a/src/SubscribeBlockTracker.ts +++ b/src/SubscribeBlockTracker.ts @@ -1,6 +1,11 @@ import type { SafeEventEmitterProvider } from '@metamask/eth-json-rpc-provider'; import SafeEventEmitter from '@metamask/safe-event-emitter'; -import type { Json, JsonRpcNotification } from '@metamask/utils'; +import { + createDeferredPromise, + type DeferredPromise, + type Json, + type JsonRpcNotification, +} from '@metamask/utils'; import getCreateRandomId from 'json-rpc-random-id'; import type { BlockTracker } from './BlockTracker'; @@ -23,7 +28,7 @@ interface SubscriptionNotificationParams { result: { number: string }; } -type InternalListener = (value: string | PromiseLike) => void; +type InternalListener = (value: string) => void; export class SubscribeBlockTracker extends SafeEventEmitter @@ -43,9 +48,9 @@ export class SubscribeBlockTracker private _subscriptionId: string | null; - readonly #internalEventListeners: (( - value: string | PromiseLike, - ) => void)[] = []; + readonly #internalEventListeners: InternalListener[] = []; + + #pendingLatestBlock?: DeferredPromise; constructor(opts: SubscribeBlockTrackerOptions = {}) { // parse + validate args @@ -78,19 +83,8 @@ export class SubscribeBlockTracker async destroy() { this._cancelBlockResetTimeout(); await this._maybeEnd(); - this.eventNames().forEach((eventName) => - this.listeners(eventName).forEach((listener) => { - if ( - this.#internalEventListeners.every( - (internalListener) => !Object.is(internalListener, listener), - ) - ) { - // @ts-expect-error this listener comes from SafeEventEmitter itself, though - // its type differs between `.listeners()` and `.removeListener()` - this.removeListener(eventName, listener); - } - }), - ); + super.removeAllListeners(); + this.#rejectPendingLatestBlock(new Error('Block tracker destroyed')); } isRunning(): boolean { @@ -105,37 +99,23 @@ export class SubscribeBlockTracker // return if available if (this._currentBlock) { return this._currentBlock; + } else if (this.#pendingLatestBlock) { + return await this.#pendingLatestBlock.promise; } - // wait for a new latest block - const latestBlock: string = await new Promise((resolve, reject) => { - // eslint-disable-next-line prefer-const - let onLatestBlockUnavailable: InternalListener; - const onLatestBlockAvailable = (value: string | PromiseLike) => { - this.#removeInternalListener(onLatestBlockAvailable); - this.#removeInternalListener(onLatestBlockUnavailable); - this.removeListener('error', onLatestBlockUnavailable); - resolve(value); - }; - onLatestBlockUnavailable = () => { - // if the block tracker is no longer running, reject - // and remove the listeners - if (!this._isRunning) { - this.#removeInternalListener(onLatestBlockAvailable); - this.#removeInternalListener(onLatestBlockUnavailable); - this.removeListener('latest', onLatestBlockAvailable); - this.removeListener('error', onLatestBlockUnavailable); - reject( - new Error('Block tracker ended before latest block was available'), - ); - } - }; - this.#addInternalListener(onLatestBlockAvailable); - this.#addInternalListener(onLatestBlockUnavailable); - this.once('latest', onLatestBlockAvailable); - this.on('error', onLatestBlockUnavailable); + + const { resolve, reject, promise } = createDeferredPromise({ + suppressUnhandledRejection: true, }); - // return newly set current block - return latestBlock; + this.#pendingLatestBlock = { resolve, reject, promise }; + + // wait for a new latest block + const onLatestBlock = (value: string) => { + this.#removeInternalListener(onLatestBlock); + this.#resolvePendingLatestBlock(value); + }; + this.#addInternalListener(onLatestBlock); + this.once('latest', onLatestBlock); + return await promise; } // dont allow module consumer to remove our internal event listeners @@ -330,6 +310,16 @@ export class SubscribeBlockTracker 1, ); } + + #resolvePendingLatestBlock(value: string) { + this.#pendingLatestBlock?.resolve(value); + this.#pendingLatestBlock = undefined; + } + + #rejectPendingLatestBlock(error: unknown) { + this.#pendingLatestBlock?.reject(error); + this.#pendingLatestBlock = undefined; + } } /** From c9e2df29faa7ac38b58cc7af0649388caefe147d Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Wed, 27 Nov 2024 13:26:27 +0100 Subject: [PATCH 14/25] Update CHANGELOG.md Co-authored-by: Mark Stacey --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19101de5..faf72f19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed -- Avoid risk of infinte retry loops when fetching new blocks ([#284](https://github.com/MetaMask/eth-block-tracker/pull/284)) +- Avoid risk of infinite retry loops when fetching new blocks ([#284](https://github.com/MetaMask/eth-block-tracker/pull/284)) ## [11.0.2] ### Fixed From f88b53d8c2202726055a1b4a3c22f81a81e8d894 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 29 Nov 2024 13:18:29 +0100 Subject: [PATCH 15/25] fix: `SubscribeBlockTracker` throws when subscription fails --- src/SubscribeBlockTracker.test.ts | 72 +++++++++++++------------------ src/SubscribeBlockTracker.ts | 2 + 2 files changed, 32 insertions(+), 42 deletions(-) diff --git a/src/SubscribeBlockTracker.test.ts b/src/SubscribeBlockTracker.test.ts index f8d8dd8b..41aa4122 100644 --- a/src/SubscribeBlockTracker.test.ts +++ b/src/SubscribeBlockTracker.test.ts @@ -278,7 +278,7 @@ describe('SubscribeBlockTracker', () => { }); METHODS_TO_ADD_LISTENER.forEach((methodToAddListener) => { - it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should never resolve if, while making the request for the latest block number, the provider throws an Error`, async () => { + it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should reject if, while making the request for the latest block number, the provider throws an Error`, async () => { recordCallsToSetTimeout({ numAutomaticCalls: 1 }); const thrownError = new Error('boom'); @@ -296,21 +296,19 @@ describe('SubscribeBlockTracker', () => { }, }, async ({ blockTracker }) => { - const promiseForCaughtError = new Promise((resolve) => { - blockTracker[methodToAddListener]('error', resolve); - }); + const listener = jest.fn(); + blockTracker[methodToAddListener]('error', listener); const promiseForLatestBlock = blockTracker[methodToGetLatestBlock](); - const caughtError = await promiseForCaughtError; - expect(caughtError).toBe(thrownError); - await expect(promiseForLatestBlock).toNeverResolve(); + await expect(promiseForLatestBlock).rejects.toThrow(thrownError); + expect(listener).toHaveBeenCalledWith(thrownError); }, ); }); - it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should never resolve if, while making the request for the latest block number, the provider throws a string`, async () => { + it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should reject if, while making the request for the latest block number, the provider throws a string`, async () => { recordCallsToSetTimeout({ numAutomaticCalls: 1 }); const thrownString = 'boom'; @@ -328,21 +326,19 @@ describe('SubscribeBlockTracker', () => { }, }, async ({ blockTracker }) => { - const promiseForCaughtError = new Promise((resolve) => { - blockTracker[methodToAddListener]('error', resolve); - }); + const listener = jest.fn(); + blockTracker[methodToAddListener]('error', listener); const promiseForLatestBlock = blockTracker[methodToGetLatestBlock](); - const caughtError = await promiseForCaughtError; - expect(caughtError).toBe(thrownString); - await expect(promiseForLatestBlock).toNeverResolve(); + await expect(promiseForLatestBlock).rejects.toBe(thrownString); + expect(listener).toHaveBeenCalledWith(thrownString); }, ); }); - it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should never resolve if, while making the request for the latest block number, the provider rejects with an error`, async () => { + it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should reject if, while making the request for the latest block number, the provider rejects with an error`, async () => { recordCallsToSetTimeout({ numAutomaticCalls: 1 }); await withSubscribeBlockTracker( @@ -357,21 +353,19 @@ describe('SubscribeBlockTracker', () => { }, }, async ({ blockTracker }) => { - const promiseForCaughtError = new Promise((resolve) => { - blockTracker[methodToAddListener]('error', resolve); - }); + const listener = jest.fn(); + blockTracker[methodToAddListener]('error', listener); const promiseForLatestBlock = blockTracker[methodToGetLatestBlock](); - const caughtError = await promiseForCaughtError; - expect(caughtError.message).toBe('boom'); - await expect(promiseForLatestBlock).toNeverResolve(); + await expect(promiseForLatestBlock).rejects.toThrow('boom'); + expect(listener).toHaveBeenCalledWith(new Error('boom')); }, ); }); - it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should never resolve if, while making the request to subscribe, the provider throws an Error`, async () => { + it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should reject if, while making the request to subscribe, the provider throws an Error`, async () => { recordCallsToSetTimeout({ numAutomaticCalls: 1 }); const thrownError = new Error('boom'); @@ -389,21 +383,19 @@ describe('SubscribeBlockTracker', () => { }, }, async ({ blockTracker }) => { - const promiseForCaughtError = new Promise((resolve) => { - blockTracker[methodToAddListener]('error', resolve); - }); + const listener = jest.fn(); + blockTracker[methodToAddListener]('error', listener); const promiseForLatestBlock = blockTracker[methodToGetLatestBlock](); - const caughtError = await promiseForCaughtError; - expect(caughtError).toBe(thrownError); - await expect(promiseForLatestBlock).toNeverResolve(); + await expect(promiseForLatestBlock).rejects.toThrow(thrownError); + expect(listener).toHaveBeenCalledWith(thrownError); }, ); }); - it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should never resolve if, while making the request to subscribe, the provider throws a string`, async () => { + it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should reject if, while making the request to subscribe, the provider throws a string`, async () => { recordCallsToSetTimeout({ numAutomaticCalls: 1 }); const thrownString = 'boom'; @@ -421,21 +413,19 @@ describe('SubscribeBlockTracker', () => { }, }, async ({ blockTracker }) => { - const promiseForCaughtError = new Promise((resolve) => { - blockTracker[methodToAddListener]('error', resolve); - }); + const listener = jest.fn(); + blockTracker[methodToAddListener]('error', listener); const promiseForLatestBlock = blockTracker[methodToGetLatestBlock](); - const caughtError = await promiseForCaughtError; - expect(caughtError).toBe(thrownString); - await expect(promiseForLatestBlock).toNeverResolve(); + await expect(promiseForLatestBlock).rejects.toBe(thrownString); + expect(listener).toHaveBeenCalledWith(thrownString); }, ); }); - it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should never resolve if, while making the request to subscribe, the provider rejects with an error`, async () => { + it(`should emit the "error" event (added via \`${methodToAddListener}\`) and should reject if, while making the request to subscribe, the provider rejects with an error`, async () => { recordCallsToSetTimeout({ numAutomaticCalls: 1 }); await withSubscribeBlockTracker( @@ -450,16 +440,14 @@ describe('SubscribeBlockTracker', () => { }, }, async ({ blockTracker }) => { - const promiseForCaughtError = new Promise((resolve) => { - blockTracker[methodToAddListener]('error', resolve); - }); + const listener = jest.fn(); + blockTracker[methodToAddListener]('error', listener); const promiseForLatestBlock = blockTracker[methodToGetLatestBlock](); - const caughtError = await promiseForCaughtError; - expect(caughtError.message).toBe('boom'); - await expect(promiseForLatestBlock).toNeverResolve(); + await expect(promiseForLatestBlock).rejects.toThrow('boom'); + expect(listener).toHaveBeenCalledWith(new Error('boom')); }, ); }); diff --git a/src/SubscribeBlockTracker.ts b/src/SubscribeBlockTracker.ts index 386ea18f..c9dd0995 100644 --- a/src/SubscribeBlockTracker.ts +++ b/src/SubscribeBlockTracker.ts @@ -264,6 +264,7 @@ export class SubscribeBlockTracker this._newPotentialLatest(blockNumber); } catch (e) { this.emit('error', e); + this.#rejectPendingLatestBlock(e); } } } @@ -275,6 +276,7 @@ export class SubscribeBlockTracker this._subscriptionId = null; } catch (e) { this.emit('error', e); + this.#rejectPendingLatestBlock(e); } } } From b3f10ec8986ae3d7b90663b596e400d864668b60 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 2 Dec 2024 11:13:02 +0100 Subject: [PATCH 16/25] test: remove broken case --- src/SubscribeBlockTracker.test.ts | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/src/SubscribeBlockTracker.test.ts b/src/SubscribeBlockTracker.test.ts index 41aa4122..b6c8915d 100644 --- a/src/SubscribeBlockTracker.test.ts +++ b/src/SubscribeBlockTracker.test.ts @@ -151,33 +151,6 @@ describe('SubscribeBlockTracker', () => { }); }); - it('should not retry failed requests after the block tracker is stopped', async () => { - recordCallsToSetTimeout({ numAutomaticCalls: 1 }); - - await withSubscribeBlockTracker( - { - provider: { - stubs: [ - { - methodName: 'eth_blockNumber', - error: 'boom', - }, - ], - }, - }, - async ({ blockTracker }) => { - const latestBlockPromise = blockTracker[methodToGetLatestBlock](); - - expect(blockTracker.isRunning()).toBe(true); - await blockTracker.destroy(); - await expect(latestBlockPromise).rejects.toThrow( - 'Block tracker destroyed', - ); - expect(blockTracker.isRunning()).toBe(false); - }, - ); - }); - it('should fetch the latest block number', async () => { recordCallsToSetTimeout(); From adde3877002ed0ade2d3a3a5ae5c5030624022cc Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 2 Dec 2024 11:20:57 +0100 Subject: [PATCH 17/25] test: add case for returning the same promise --- src/SubscribeBlockTracker.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/SubscribeBlockTracker.test.ts b/src/SubscribeBlockTracker.test.ts index b6c8915d..5ae655a0 100644 --- a/src/SubscribeBlockTracker.test.ts +++ b/src/SubscribeBlockTracker.test.ts @@ -142,6 +142,21 @@ describe('SubscribeBlockTracker', () => { }); }); + it('should return the same promise if called multiple times', async () => { + recordCallsToSetTimeout(); + + await withSubscribeBlockTracker(async ({ blockTracker }) => { + const promiseToGetLatestBlock1 = + blockTracker[methodToGetLatestBlock](); + const promiseToGetLatestBlock2 = + blockTracker[methodToGetLatestBlock](); + + expect(promiseToGetLatestBlock1).toStrictEqual( + promiseToGetLatestBlock2, + ); + }); + }); + it('should stop the block tracker automatically after its promise is fulfilled', async () => { recordCallsToSetTimeout(); From 532ee3d6d0225983138b42552ff3785fd71e0fc2 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 2 Dec 2024 14:05:56 +0100 Subject: [PATCH 18/25] test: add coverage for `PollingBlockTracker` --- src/PollingBlockTracker.test.ts | 83 +++++++++++++++++++++++++++++++++ src/PollingBlockTracker.ts | 8 +--- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/src/PollingBlockTracker.test.ts b/src/PollingBlockTracker.test.ts index 2511dd34..ff54e0b6 100644 --- a/src/PollingBlockTracker.test.ts +++ b/src/PollingBlockTracker.test.ts @@ -212,6 +212,62 @@ describe('PollingBlockTracker', () => { ); }); + it('should return the same promise if called multiple times', async () => { + await withPollingBlockTracker(async ({ blockTracker }) => { + const promiseToGetLatestBlock1 = blockTracker.getLatestBlock(); + const promiseToGetLatestBlock2 = blockTracker.getLatestBlock(); + + expect(promiseToGetLatestBlock1).toStrictEqual( + promiseToGetLatestBlock2, + ); + }); + }); + + it('should return a promise that resolves when a new block is available', async () => { + recordCallsToSetTimeout(); + + await withPollingBlockTracker( + { + provider: { + stubs: [ + { + methodName: 'eth_blockNumber', + result: '0x1', + }, + ], + }, + }, + async ({ blockTracker }) => { + expect(await blockTracker.getLatestBlock()).toBe('0x1'); + }, + ); + }); + + it('should resolve all returned promises when a new block is available', async () => { + recordCallsToSetTimeout(); + + await withPollingBlockTracker( + { + provider: { + stubs: [ + { + methodName: 'eth_blockNumber', + result: '0x1', + }, + ], + }, + }, + async ({ blockTracker }) => { + const promises = [ + blockTracker.getLatestBlock(), + blockTracker.getLatestBlock(), + ]; + + expect(await Promise.all(promises)).toStrictEqual(['0x1', '0x1']); + }, + ); + }); + it('request the latest block number with `skipCache: true` if the block tracker was initialized with `setSkipCacheFlag: true`', async () => { recordCallsToSetTimeout(); @@ -602,6 +658,33 @@ describe('PollingBlockTracker', () => { }); }); + it('should return the same promise if called multiple times', async () => { + await withPollingBlockTracker( + { + provider: { + stubs: [ + { + methodName: 'eth_blockNumber', + result: '0x0', + }, + { + methodName: 'eth_blockNumber', + result: '0x1', + }, + ], + }, + }, + async ({ blockTracker }) => { + const promiseToCheckLatestBlock1 = blockTracker.checkForLatestBlock(); + const promiseToCheckLatestBlock2 = blockTracker.checkForLatestBlock(); + + expect(promiseToCheckLatestBlock1).toStrictEqual( + promiseToCheckLatestBlock2, + ); + }, + ); + }); + it('should fetch the latest block number', async () => { recordCallsToSetTimeout(); diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index 815d0806..9ff090a7 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -126,7 +126,8 @@ export class PollingBlockTracker // wait for a new latest block const onLatestBlock = (value: string) => { this.#removeInternalListener(onLatestBlock); - this.#resolvePendingLatestBlock(value); + resolve(value); + this.#pendingLatestBlock = undefined; }; this.#addInternalListener(onLatestBlock); this.once('latest', onLatestBlock); @@ -373,11 +374,6 @@ export class PollingBlockTracker ); } - #resolvePendingLatestBlock(value: string) { - this.#pendingLatestBlock?.resolve(value); - this.#pendingLatestBlock = undefined; - } - #rejectPendingLatestBlock(error: unknown) { this.#pendingLatestBlock?.reject(error); this.#pendingLatestBlock = undefined; From 1de91129f4b715c1f477b52123331d760af4c2e6 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Mon, 2 Dec 2024 15:40:06 +0100 Subject: [PATCH 19/25] test: add coverage for `SubscribeBlockTracker` --- src/PollingBlockTracker.ts | 4 ++-- src/SubscribeBlockTracker.test.ts | 13 +++++++++++++ src/SubscribeBlockTracker.ts | 12 ++++-------- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/PollingBlockTracker.ts b/src/PollingBlockTracker.ts index 9ff090a7..b0ad70d2 100644 --- a/src/PollingBlockTracker.ts +++ b/src/PollingBlockTracker.ts @@ -61,7 +61,7 @@ export class PollingBlockTracker readonly #internalEventListeners: InternalListener[] = []; - #pendingLatestBlock?: DeferredPromise; + #pendingLatestBlock?: Omit, 'resolve'>; constructor(opts: PollingBlockTrackerOptions = {}) { // parse + validate args @@ -121,7 +121,7 @@ export class PollingBlockTracker const { promise, resolve, reject } = createDeferredPromise({ suppressUnhandledRejection: true, }); - this.#pendingLatestBlock = { promise, resolve, reject }; + this.#pendingLatestBlock = { reject, promise }; // wait for a new latest block const onLatestBlock = (value: string) => { diff --git a/src/SubscribeBlockTracker.test.ts b/src/SubscribeBlockTracker.test.ts index 5ae655a0..fa04c776 100644 --- a/src/SubscribeBlockTracker.test.ts +++ b/src/SubscribeBlockTracker.test.ts @@ -166,6 +166,19 @@ describe('SubscribeBlockTracker', () => { }); }); + it('should reject the returned promise if the block tracker is destroyed in the meantime', async () => { + await withSubscribeBlockTracker(async ({ blockTracker }) => { + const promiseToGetLatestBlock = + blockTracker[methodToGetLatestBlock](); + await blockTracker.destroy(); + + await expect(promiseToGetLatestBlock).rejects.toThrow( + 'Block tracker destroyed', + ); + expect(blockTracker.isRunning()).toBe(false); + }); + }); + it('should fetch the latest block number', async () => { recordCallsToSetTimeout(); diff --git a/src/SubscribeBlockTracker.ts b/src/SubscribeBlockTracker.ts index c9dd0995..f3cc2241 100644 --- a/src/SubscribeBlockTracker.ts +++ b/src/SubscribeBlockTracker.ts @@ -50,7 +50,7 @@ export class SubscribeBlockTracker readonly #internalEventListeners: InternalListener[] = []; - #pendingLatestBlock?: DeferredPromise; + #pendingLatestBlock?: Omit, 'resolve'>; constructor(opts: SubscribeBlockTrackerOptions = {}) { // parse + validate args @@ -106,12 +106,13 @@ export class SubscribeBlockTracker const { resolve, reject, promise } = createDeferredPromise({ suppressUnhandledRejection: true, }); - this.#pendingLatestBlock = { resolve, reject, promise }; + this.#pendingLatestBlock = { reject, promise }; // wait for a new latest block const onLatestBlock = (value: string) => { this.#removeInternalListener(onLatestBlock); - this.#resolvePendingLatestBlock(value); + resolve(value); + this.#pendingLatestBlock = undefined; }; this.#addInternalListener(onLatestBlock); this.once('latest', onLatestBlock); @@ -313,11 +314,6 @@ export class SubscribeBlockTracker ); } - #resolvePendingLatestBlock(value: string) { - this.#pendingLatestBlock?.resolve(value); - this.#pendingLatestBlock = undefined; - } - #rejectPendingLatestBlock(error: unknown) { this.#pendingLatestBlock?.reject(error); this.#pendingLatestBlock = undefined; From eaba6e8f3fe7c5944642bed8b999922e05c12385 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 3 Dec 2024 15:19:00 +0100 Subject: [PATCH 20/25] test: remove redundant tests --- src/PollingBlockTracker.test.ts | 11 ----------- src/SubscribeBlockTracker.test.ts | 15 --------------- 2 files changed, 26 deletions(-) diff --git a/src/PollingBlockTracker.test.ts b/src/PollingBlockTracker.test.ts index ff54e0b6..6b889a33 100644 --- a/src/PollingBlockTracker.test.ts +++ b/src/PollingBlockTracker.test.ts @@ -212,17 +212,6 @@ describe('PollingBlockTracker', () => { ); }); - it('should return the same promise if called multiple times', async () => { - await withPollingBlockTracker(async ({ blockTracker }) => { - const promiseToGetLatestBlock1 = blockTracker.getLatestBlock(); - const promiseToGetLatestBlock2 = blockTracker.getLatestBlock(); - - expect(promiseToGetLatestBlock1).toStrictEqual( - promiseToGetLatestBlock2, - ); - }); - }); - it('should return a promise that resolves when a new block is available', async () => { recordCallsToSetTimeout(); diff --git a/src/SubscribeBlockTracker.test.ts b/src/SubscribeBlockTracker.test.ts index fa04c776..0a3deb57 100644 --- a/src/SubscribeBlockTracker.test.ts +++ b/src/SubscribeBlockTracker.test.ts @@ -142,21 +142,6 @@ describe('SubscribeBlockTracker', () => { }); }); - it('should return the same promise if called multiple times', async () => { - recordCallsToSetTimeout(); - - await withSubscribeBlockTracker(async ({ blockTracker }) => { - const promiseToGetLatestBlock1 = - blockTracker[methodToGetLatestBlock](); - const promiseToGetLatestBlock2 = - blockTracker[methodToGetLatestBlock](); - - expect(promiseToGetLatestBlock1).toStrictEqual( - promiseToGetLatestBlock2, - ); - }); - }); - it('should stop the block tracker automatically after its promise is fulfilled', async () => { recordCallsToSetTimeout(); From b9da4b87c734c87aae790d11b3820e9a4ab200ec Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 3 Dec 2024 15:24:01 +0100 Subject: [PATCH 21/25] test: check promises return for `SubscribeBlockTracker` --- src/SubscribeBlockTracker.test.ts | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/SubscribeBlockTracker.test.ts b/src/SubscribeBlockTracker.test.ts index 0a3deb57..0fa51e40 100644 --- a/src/SubscribeBlockTracker.test.ts +++ b/src/SubscribeBlockTracker.test.ts @@ -151,6 +151,31 @@ describe('SubscribeBlockTracker', () => { }); }); + it('should resolve all returned promises when a new block is available', async () => { + recordCallsToSetTimeout(); + + await withSubscribeBlockTracker( + { + provider: { + stubs: [ + { + methodName: 'eth_blockNumber', + result: '0x1', + }, + ], + }, + }, + async ({ blockTracker }) => { + const promises = [ + blockTracker.getLatestBlock(), + blockTracker.getLatestBlock(), + ]; + + expect(await Promise.all(promises)).toStrictEqual(['0x1', '0x1']); + }, + ); + }); + it('should reject the returned promise if the block tracker is destroyed in the meantime', async () => { await withSubscribeBlockTracker(async ({ blockTracker }) => { const promiseToGetLatestBlock = From 33db227f1f9fc6a76ff01f9b63e76ce4eef992c1 Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Tue, 3 Dec 2024 17:44:06 +0100 Subject: [PATCH 22/25] rename test case Co-authored-by: Elliot Winkler --- src/PollingBlockTracker.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PollingBlockTracker.test.ts b/src/PollingBlockTracker.test.ts index 6b889a33..812863c5 100644 --- a/src/PollingBlockTracker.test.ts +++ b/src/PollingBlockTracker.test.ts @@ -185,7 +185,7 @@ describe('PollingBlockTracker', () => { ); }); - it('should not retry failed requests after the block tracker is stopped', async () => { + it('should return a promise that rejects if the request for the block number fails and the block tracker is then stopped', async () => { recordCallsToSetTimeout({ numAutomaticCalls: 1 }); await withPollingBlockTracker( From efceaf7e3ca34fcc4d4eb0268a0fd339dc423c01 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 3 Dec 2024 18:13:13 +0100 Subject: [PATCH 23/25] test: spy on `provider.request` --- src/PollingBlockTracker.test.ts | 34 +++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/PollingBlockTracker.test.ts b/src/PollingBlockTracker.test.ts index 812863c5..4468bd17 100644 --- a/src/PollingBlockTracker.test.ts +++ b/src/PollingBlockTracker.test.ts @@ -212,6 +212,40 @@ describe('PollingBlockTracker', () => { ); }); + it('should not retry failed requests after the block tracker is stopped', async () => { + recordCallsToSetTimeout({ numAutomaticCalls: 1 }); + + await withPollingBlockTracker( + { + provider: { + stubs: [ + { + methodName: 'eth_blockNumber', + error: 'boom', + }, + ], + }, + }, + async ({ blockTracker, provider }) => { + const requestSpy = jest.spyOn(provider, 'request'); + + const latestBlockPromise = blockTracker.getLatestBlock(); + await blockTracker.destroy(); + + await expect(latestBlockPromise).rejects.toThrow( + 'Block tracker destroyed', + ); + expect(requestSpy).toHaveBeenCalledTimes(1); + expect(requestSpy).toHaveBeenCalledWith({ + jsonrpc: '2.0', + id: expect.any(Number), + method: 'eth_blockNumber', + params: [], + }); + }, + ); + }); + it('should return a promise that resolves when a new block is available', async () => { recordCallsToSetTimeout(); From bc6e7d6d6a3c575e637297d7b9a537babb15311d Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 3 Dec 2024 18:18:21 +0100 Subject: [PATCH 24/25] edit changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index faf72f19..7b899c8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed - Avoid risk of infinite retry loops when fetching new blocks ([#284](https://github.com/MetaMask/eth-block-tracker/pull/284)) + - When the provider returns an error and `PollingBlockTracker` or `SubscribeBlockTracker` is destroyed, the promise returned by the `getLatestBlock` method will be rejected. ## [11.0.2] ### Fixed From 821bae51cc4bf65d8dad65fec1e254b844812617 Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Tue, 3 Dec 2024 18:24:34 +0100 Subject: [PATCH 25/25] test: set automatic timer calls to 2 --- src/PollingBlockTracker.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PollingBlockTracker.test.ts b/src/PollingBlockTracker.test.ts index 4468bd17..d826bc60 100644 --- a/src/PollingBlockTracker.test.ts +++ b/src/PollingBlockTracker.test.ts @@ -213,7 +213,7 @@ describe('PollingBlockTracker', () => { }); it('should not retry failed requests after the block tracker is stopped', async () => { - recordCallsToSetTimeout({ numAutomaticCalls: 1 }); + recordCallsToSetTimeout({ numAutomaticCalls: 2 }); await withPollingBlockTracker( {