fix: internal listeners infinite retry loop

CHANGELOG.md

@@ -5,6 +5,9 @@ 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 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

src/PollingBlockTracker.test.ts

@@ -185,6 +185,112 @@ describe('PollingBlockTracker', () => {
       );
     });
 
+    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(
+        {
+          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 destroyed',
+          );
+          expect(blockTracker.isRunning()).toBe(false);
+        },
+      );
+    });
+
+    it('should not retry failed requests after the block tracker is stopped', async () => {
+      recordCallsToSetTimeout({ numAutomaticCalls: 2 });
+
+      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();
+
+      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();
 
@@ -575,6 +681,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();
 

src/PollingBlockTracker.ts

@@ -1,6 +1,11 @@
 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,
+  type DeferredPromise,
+  getErrorMessage,
+  type JsonRpcRequest,
+} from '@metamask/utils';
 import getCreateRandomId from 'json-rpc-random-id';
 
 import type { BlockTracker } from './BlockTracker';
@@ -10,8 +15,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 {
@@ -28,6 +31,8 @@ interface ExtendedJsonRpcRequest extends JsonRpcRequest<[]> {
   skipCache?: boolean;
 }
 
+type InternalListener = (value: string) => void;
+
 export class PollingBlockTracker
   extends SafeEventEmitter
   implements BlockTracker
@@ -54,6 +59,10 @@ export class PollingBlockTracker
 
   private readonly _setSkipCacheFlag: boolean;
 
+  readonly #internalEventListeners: InternalListener[] = [];
+
+  #pendingLatestBlock?: Omit<DeferredPromise<string>, 'resolve'>;
+
   constructor(opts: PollingBlockTrackerOptions = {}) {
     // parse + validate args
     if (!opts.provider) {
@@ -90,6 +99,7 @@ export class PollingBlockTracker
     this._cancelBlockResetTimeout();
     this._maybeEnd();
     super.removeAllListeners();
+    this.#rejectPendingLatestBlock(new Error('Block tracker destroyed'));
   }
 
   isRunning(): boolean {
@@ -104,13 +114,24 @@ export class PollingBlockTracker
     // return if available
     if (this._currentBlock) {
       return this._currentBlock;
+    } else if (this.#pendingLatestBlock) {
+      return await this.#pendingLatestBlock.promise;
     }
+
+    const { promise, resolve, reject } = createDeferredPromise<string>({
+      suppressUnhandledRejection: true,
+    });
+    this.#pendingLatestBlock = { reject, promise };
+
     // wait for a new latest block
-    const latestBlock: string = await new Promise((resolve) =>
-      this.once('latest', resolve),
-    );
-    // return newly set current block
-    return latestBlock;
+    const onLatestBlock = (value: string) => {
+      this.#removeInternalListener(onLatestBlock);
+      resolve(value);
+      this.#pendingLatestBlock = undefined;
+    };
+    this.#addInternalListener(onLatestBlock);
+    this.once('latest', onLatestBlock);
+    return await promise;
   }
 
   // dont allow module consumer to remove our internal event listeners
@@ -179,9 +200,17 @@ export class PollingBlockTracker
   }
 
   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) {
@@ -333,6 +362,22 @@ export class PollingBlockTracker
       this._pollingTimeout = undefined;
     }
   }
+
+  #addInternalListener(listener: InternalListener) {
+    this.#internalEventListeners.push(listener);
+  }
+
+  #removeInternalListener(listener: InternalListener) {
+    this.#internalEventListeners.splice(
+      this.#internalEventListeners.indexOf(listener),
+      1,
+    );
+  }
+
+  #rejectPendingLatestBlock(error: unknown) {
+    this.#pendingLatestBlock?.reject(error);
+    this.#pendingLatestBlock = undefined;
+  }
 }
 
 /**