fix: internal listeners infinite retry loop #284
Conversation
|
It would be good to cover this with tests |
Co-authored-by: Mark Stacey <[email protected]>
This reverts commit eb5514d.
src/PollingBlockTracker.ts
Outdated
| this.#addInternalListener(onLatestBlockAvailable); | ||
| this.#addInternalListener(onLatestBlockUnavailable); | ||
| this.once('latest', onLatestBlockAvailable); | ||
| this.on('error', onLatestBlockUnavailable); |
There was a problem hiding this comment.
Hmm, I'm thinking that this would not cover the scenario where the block tracker is destroyed before fetching the first block and without throwing errors.
We can probably listen to _ended instead
There was a problem hiding this comment.
It looks like _ended could present other issues, because of the weird order of events received:
_startis emitted- Latest block is fetched from the provider
_endedevent listener is executedlatestevent listener is executed
There was a problem hiding this comment.
If I understand correctly, because getLatestBlock now relies on latest or error to fire in order to resolve the promise it creates, if that never happens because the fetch call never happens, then getLatestBlock will just hang, even if destroy is called?
I think listening to _ended could make sense. Perhaps we add a check in onLatestBlockAvailable that only resolves the promise if the block runner is still running?
There was a problem hiding this comment.
I am slow today :/ It seems that Mark's solution in #286 may solve that case, because it grants destroy access to the promise that getLatestBlock creates, and we can just force-reject it if it hasn't resolved yet.
src/PollingBlockTracker.ts
Outdated
| @@ -89,7 +91,19 @@ export class PollingBlockTracker | |||
| async destroy() { | |||
| this._cancelBlockResetTimeout(); | |||
| this._maybeEnd(); | |||
| super.removeAllListeners(); | |||
| this.eventNames().forEach((eventName) => | |||
There was a problem hiding this comment.
Do we want to explain why excluding internal listeners are necessary? Perhaps something like:
| this.eventNames().forEach((eventName) => | |
| // The `getLatestBlock` method waits for the latest block to be fetched in | |
| // the next iteration of the polling loop. It does this by listening to the | |
| // `latest` event, and it handles errors that occur during fetching by | |
| // listening to the `error` event. Because `getLatestBlock` is actively | |
| // relying on these listeners to get called in order to resolve or reject a | |
| // promise, we don't want to remove them (otherwise the polling loop will | |
| // run forever and the promise will never get fulfilled). We will handle | |
| // removing them manually. | |
| this.eventNames().forEach((eventName) => |
There was a problem hiding this comment.
By the way, I was playing around with this PR and it seems that if you revert the changes to destroy and _getBlockTrackerEventCount, then your test still works. It seems that listening to error solved at least one bug. But perhaps there is another test we need to write to verify the endless loop bug? I can take another look tomorrow to see what that would be.
| @@ -185,6 +185,33 @@ describe('PollingBlockTracker', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| it('should not retry failed requests after the block tracker is stopped', async () => { | |||
There was a problem hiding this comment.
Hmm, this test does not seem to test that requests are not retried. I believe what it tests is that the promise that getLatestBlock returns eventually resolves or is rejected. Should we be more clear here? Something like:
| 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 () => { |
Or do we have a way to test that requests are not retried?
There was a problem hiding this comment.
Well, perhaps the original test name is fine. I guess the test would fail if the polling loop continued because we are using numAutomaticCalls: 1. Maybe the new test name I am suggesting is appropriate for the case you mentioned above, or something.
* 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
Our block tracker implementation relies on the presence of listeners to establish whether the polling should be continued or stopped.
However, an internal listener is being added in the
getLatestBlockmethod, which will count as the other external listeners and will prevent the instance from stopping fetching new blocks. This creates an infinite loop in case the network is unreachable, because of the retry mechanism.This PR aims to fix this behavior by keeping track of internal listeners' references in order to exclude them from the listener's count
Fixes #163