Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Figure out a way to completely stop a polling block tracker in tests #129

Open
mcmire opened this issue Feb 7, 2023 · 0 comments
Open

Comments

@mcmire
Copy link
Contributor

mcmire commented Feb 7, 2023

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. 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 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 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants