PollingBlockTracker runs endlessly when started by an internal event

[Gudahtt]

Recently we fixed a bug (in #284) that caused the block tracker to run endlessly upon failure. In fixing this, we accidentally introduced a new bug: when the polling block tracker is started by an internal function, it never stops.

The previous bug was fixed by ensuring that we ignore internal functions when deciding whether to stop the polling upon a listener being removed. But internal listeners still start the polling, and there is no listener removal, there is no opportunity for it to shut down.

[Gudahtt]

I discovered this when trying to reproduce this bug on extension: MetaMask/metamask-extension#28868 (on the main branch).

At first I noticed that there was an account tracking controller polling token bug; the poller was kept alive by that controller. But when I commented out the code to start that poll, it was still kept alive due to this bug.

[Gudahtt]

We should be able to fix this by checking at the end of each loop to see if we should stop or not.

Alternatively, maybe we could further separate the internal calls from the normal polling loop, so they don't rely on isRunning? That sounds like more work, so maybe not the best solution for now, but something to consider.

[mcmire]

I spoke with Mark on Friday about this and it seems like the bug may be different than represented here.

What we're seeing is that getLatestBlock never resolves if the fetch call always fails. This behavior has existed for a long time.

Quoting Mark:

Potential strategies to mitigate that:

• Call _updateLatestBlock if not running, otherwise wait for latest OR polling stop (and if polling stop is triggered, then call _updateLatestBlock)
  • Slightly reduced performance because now the network call is happening slightly sooner than it used to, sometimes? Pretty marginal though
• Call _updateLatestBlock in all cases, but reset the polling loop (if it's active)
  • Slightly reduced performance because the network call is happening sooner than it used to most of the time, but still marginal.
• Use the first strategy, but store a "last fetched" time, so that we can delay the _updateLatestBlock call if it's within an interval from the last call. We can also keep this Promise around so that if the polling starts again, it will await the pending call before starting
  • This is optimal in terms of preventing any additional network calls

He thought there should be a simpler solution and said:

Storing the pending call for the current poller would do it.

e.g. have a checkLatestBlockAfterINterval Promise that is set at the beginning of a polling interval. Then if getLatestBlock is called, do something like

if (this.#checkLatestBlockAferInterval) {
  return await #checkLatestBlockAfterInterval;
}
this.#checkLatestBlock = this._updateLatestBlock
return await this.#checkLatestBlock

Then inside of the poller, we do something like this:

// this would only happen if the poll is stopped and started in quick succession I think
if (this.#checkLatestBlockAferInterval) {
  await #checkLatestBlockAfterInterval;
} else if (this.#checkLatestBlock) {
  await this.#checkLatestBlock;
}
if (!this._isRunning) {
  return;
}
this.#checkLatestBlockAfterInterval = this.#checkLatestBlockAfterInterval()
await this.#checkLatestBlockAfterInterval
// recursive call self

and then #checkLatestBlockAfterInterval would be something like

async function #checkLatestBlockAfterInterval() {
  await new Promise(resolve => setTimeout(resolve, pollingInterval));
  if (this.#checkLatestBlock) {
    await this.#checkLatestBlock;
  }
  this.#checkLatestBlock = this._updateLatestBlock();
  await this.#checkLatestBlock;
}

After that he said:

I see it doesn't prevent successive getLatestBlock calls while polling is stopped though 😅 . Oof. Not optimal then. There must be a similar solution that is optimal.... Maybe storing the "waitForInterval" promise as separate, not as containing the block update. Then we could hang a non-blocking "wait for interval" after the getLatestBlock call.

The last he said was that he was leaning toward the "debounce" strategy as being the simplest.