fix: modify unsubscribe cleanup routine and tests #84
+98
−56
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ignore exceptions (other than CancelledError) if uninstallation of the filter fails. If it's the last step in the subscription cleanup, then filter changes for this filter will no longer be polled so if the filter continues to live on in geth for whatever reason, then it doesn't matter.
- add random port - add more logging
This includes a number of fixes: - `CancelledError` is now caught inside of `getChanges`. This was causing conditions during `subscriptions.close`, where the `CancelledError` would get consumed by the `except CatchableError`, if there was an ongoing `poll` happening at the time of close. - After creating a new filter inside of `getChanges`, the new filter is polled for changes before returning. - `getChanges` also does not swallow `CatchableError` by returning an empty array, and instead re-raises the error if it is not `filter not found`. - The tests were simplified by accessing the private fields of `PollingSubscriptions`. That way, there wasn't a race condition for the `newFilterId` counter inside of the mock. - The `MockRpcHttpServer` was simplified by keeping track of the active filters only, and invalidation simply removes the filter. The tests then only needed to rely on the fact that the filter id changed in the mapping. - Because of the above changes, we no longer needed to sleep inside of the tests, so the sleeps were removed, and the polling interval could be changed to 1ms, which not only makes the tests faster, but would further highlight any race conditions if present.
AuHau
approved these changes
Oct 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #79
Modifying the cleanup:
To fix the subscription filter tests:
CancelledErroris now caught inside ofgetChanges. This was causing conditions duringsubscriptions.close, where theCancelledErrorwould get consumed by theexcept CatchableError, if there was an ongoingpollhappening at the time of close.getChanges, the new filter is polled for changes before returning.getChangesalso does not swallowCatchableErrorby returning an empty array, and instead re-raises the error if it is notfilter not found.eth_getFilterChanges. IfJNullwas received, then an empty array is returned.PollingSubscriptions. That way, there wasn't a race condition for thenewFilterIdcounter inside of the mock.MockRpcHttpServerwas simplified by keeping track of the active filters only, and invalidation simply removes the filter. The tests then only needed to rely on the fact that the filter id changed in the mapping.Co-authored-by: Adam Uhlíř [email protected]