[eth-block-tracker] Figure out a way to completely stop a polling block tracker in tests
- Dominant language
- TypeScript
- Stars
- 413
- Forks
- 308
- Avg merge
- 1d 4h
- Merged PRs (30d)
- 253
Description
Since the block tracker runs out of band, it has the potential to wreak havoc on tests, especially if we are mocking requests using Nock and/or time using fake timers. We have to be very careful in these situations that requests to the provider do not start in one test and finish in another test.
We might think that we can stop the block tracker at the end of each test to ensure that this doesn't occur, but this doesn't always work.
First, it's important to understand how the block tracker works:
* The block tracker is started automatically whenever the `latest` or `sync` event is listened to (these events are aliases for each other).
* In the case of the polling block tracker, which is used much more commonly than the subscribe block tracker, a loop is started.
* The method that starts this loop is called `_synchronize`, and although it is an async method, [it is never awaited](https://github.com/MetaMask/eth-block-tracker/blob/0ace5a3b6b64f686acd08cb2525ea4e112c4a202/src/PollingBlockTracker.ts#L62). That means the loop happens out of band.
* Inside this loop, the block tracker makes a request for the latest block, and then it waits for a predetermined amount of time, using `setTimeout` wrapped in a promise. When the promise resolves, the loop continues.
* The block tracker is stopped automatically when the last instance of `latest`/`sync` is removed. The most common way this happens is when using `getLatestBlock`, where the `latest` event is listened to and is then removed once the event occurs.
* The block tracker can also get stopped manually via the `destroy` method, which we employ in tests.
We mentioned above that stopping the block tracker doesn't work as expected, but why? There are really two problems here:
1. The block tracker doesn't get immediately stopped. Whether the block tracker is automatically or manually stopped, the [`_maybeEnd` method in BaseBlockTracker](https://github.com/MetaMask/eth-block-tracker/blob/0ace5a3b6b64f686acd08cb2525ea4e112c4a202/src/BaseBlockTracker.ts#L129) is called. This method does a few things, but it will primarily set the `_isRunning` property to false. As [the loop in PollingBlockTracker checks to see if `_isRunning` is true](https://github.com/MetaMask/eth-block-tracker/blob/0ace5a3b6b64f686acd08cb2525ea4e112c4a202/src/PollingBlockTracker.ts#L70) to determine whether to keep iterating, once it is set to false, the loop should stop on the next iteration. The problem is that if we are faking timers in a test, this never actually happens, because the `setTimeout` call that occurs at the end of an iteration will never occur. What ends up happening in a test suite, then, is that all of these block tracker objects get created and started but never ended, so they're just sitting around in memory along with all of the pending `setTimeout` calls. These need to be cleared in order to guarantee that no additional requests are made that we haven't accounted for. Also, if the block tracker is in the middle of making a request when it's stopped, that request will go through. So ideally, we need to not only cancel the `setTimeout` call via `clearTimeout` but we also need to use an AbortController to cancel the pending request.
2. When the block tracker is automatically stopped, we have no way of waiting until it's really stopped. This is because the stopping occurs via `_onRemoveListener`. Although it calls `_maybeEnd` and `_maybeEnd` is an async method, it cannot await this method, because `_onRemoveListener` is called when the `removeListener` event occurs, and events aren't handled asynchronously. This makes sense from an event perspective, but it's a pretty glaring design flaw. There's really no reason to use EventEmitter for the block tracker, as we only use ~two~ one event. It would be much easier if instead of having consumers subscribe to the `latest`/`sync` event they just call `.onLatest` or something like that. That way we wouldn't also have to do things like [override the `removeAllListeners` method to ensure that consumers can't remove our internal events](https://github.com/MetaMask/eth-block-tracker/blob/0ace5a3b6b64f686acd08cb2525ea4e112c4a202/src/BaseBlockTracker.ts#L68).
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with src/BaseBlockTracker.ts, especially _maybeEnd and the remove-listener path, then read src/PollingBlockTracker.ts around _synchronize and its timer loop. Reproduce the behavior in the existing block-tracker tests, if present, using fake timers and mocked requests. Done should ensure stopped trackers leave no pending timers or requests and that tests can wait for automatic shutdown.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- testing-qa
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100