rpcclient/WS: fix data race on concurrent (un)subscription #3532
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
Every client's (Un)Subscription call does two things: an RPC call and a subscription map lock (two of maps currently). If we imagine that there is one routine that tries to subscribe (A) and one routine that tries to unsubscribe (B), the following sequence can happen: 0. Current number of subscriptions is X 1. B does an RPC and makes number of subscriptions X-1 2. A does an RPC and makes number of subscriptions X again 3. A holds subscription locks and rewrites client's subscription state (subscription with ID X now points to a different channel; channel that was registered by B is lost and is not related to any real subscription but is still included in the `receivers` map) 4. B holds subscription locks and drops subscription X (first, it is an error and we have just lost a subscription that we think was made successfully second, we have lost a channel in the `receivers` map, and no corresponding subscription points to it) 5. X subscription is received by the WS client (in practice it is a new block, 100ms, quite often to be sure this issue happens every hour), we range through the receivers, see no corresponding subscription, and panic. Closes #3093. Signed-off-by: Pavel Karpy <[email protected]>
|
@roman-khimov, we can try this. It is this comment's implementation. We have tried it in our loadings, no panic was found. Can be acceptable for a patch release. Does not stop us from creating a new issue that fixes it with a design change on the server side. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3532 +/- ##
==========================================
- Coverage 86.19% 86.08% -0.12%
==========================================
Files 331 330 -1
Lines 38494 38658 +164
==========================================
+ Hits 33179 33277 +98
- Misses 3785 3839 +54
- Partials 1530 1542 +12 ☔ View full report in Codecov by Sentry. |
roman-khimov
approved these changes
Jul 26, 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.
Every client's (Un)Subscription call does two things: an RPC call and a subscription map lock (two of maps currently). If we imagine that there is one routine that tries to subscribe (A) and one routine that tries to unsubscribe (B), the following sequence can happen:
receiversmap)receiversmap, and no corresponding subscription points to it)Closes #3093.