fix(bitswap/client/msgq): prevent duplicate requests #691
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
Wondertan
commented
Oct 17, 2024
| if mq.bcstWants.sent.Len() == 0 && mq.peerWants.sent.Len() == 0 { | ||
| return false | ||
| mq.rebroadcastIntervalLk.RLock() | ||
| rebroadcastInterval := mq.rebroadcastInterval |
There was a problem hiding this comment.
Alternatively, this could be a different new parameter/constant
|
I tested this on a k8s cluster and with a local node connected to it. It works as expected, but I believe this would benefit a lot from a proper test. Unfortunately, I can't allocate time to writing one. It's not that straightforward. |
|
For context, I detect duplicates with a custom multihash that logs out when the same data is hashed again. This essentially uncovered #690, and this issue |
Wondertan
force-pushed
the
message-queue-duplicates
branch
2 times, most recently
from
070d0e4
to
d193c2f
Compare
Previously, in-progress requests could be re-requested again during periodic rebroadcast. The queue requests, and while awaiting response, the rebroadcast event happens. Rebroadcast event changes previosly sent WANTs to pending and sends them again in a new message. The solution here is to ensure WANT was in sent status for long enough, before bringing it back to pending. This utilizes existing `sendAt` map which tracks when every CID was sent.
Wondertan
force-pushed
the
message-queue-duplicates
branch
from
d193c2f
to
9020b71
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
@@ Coverage Diff @@
## main #691 +/- ##
==========================================
+ Coverage 60.36% 60.39% +0.03%
==========================================
Files 243 243
Lines 31034 31048 +14
==========================================
+ Hits 18734 18752 +18
+ Misses 10634 10633 -1
+ Partials 1666 1663 -3
|
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.
Previously, in-progress requests could be re-requested again during periodic rebroadcast. The queue requests, and while awaiting a response, the rebroadcast event happens. Rebroadcast event changes previously sent WANTs to pending and sends them again in a new message, duplicating some WANT requests.
The solution here is to ensure WANT was in sent status for long enough before bringing it back to pending. This utilizes the existing
sendAtmap, which tracks when every CID was sent. Then, on every event, it compares if the message was around longer thanrebroadcastInterval