Attempt to fix spuriously failing test "dont_make_announcements_while_blocks_are_being_sent" #1309
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
ImplOfAnImpl
force-pushed
the
attempt_to_fix_spuriously_failing_test
branch
from
a592b91 to
d58c313
Compare
ImplOfAnImpl
requested review from
muursh,
erubboli,
iljakuklic,
TheQuantumPhysicist and
azarovh
as code owners
Merged
ImplOfAnImpl
commented
Oct 30, 2023
TheQuantumPhysicist
approved these changes
Oct 30, 2023
|
Please merge this only when we're ready to merge its base to master. |
ImplOfAnImpl
force-pushed
the
p2p_v2_issues
branch
3 times, most recently
from
ce50a48 to
3d3b1d9
Compare
…blocks_are_being_sent
…e_blocks_are_being_sent back to 100
…blocks_are_being_sent; use explicit delay.
ImplOfAnImpl
force-pushed
the
attempt_to_fix_spuriously_failing_test
branch
from
6912fab to
46a722b
Compare
…blocks are requested out of order
Fixes in p2p related to blocks order
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.
The test
dont_make_announcements_while_blocks_are_being_sentwas added in PR #1306; it was failing spuriously on my local machine from time to time (depending on the CPU load), which forced me to increase the number of initial blocks from 10 to 100. But then it failed on CI anyway.The behaviour that it checks is basically a small optimization, so I was seriously considering just removing the test. However I decided to try to make it more reliable. I tries several approaches; the final "solution" (which I'm not particularly fond of, to be honest) was to make
MessagingService::send_messageasync, so that test implementations of it could use a bounded channel. The problematic test now uses the channel capacity of 1, and can usesleepto slow down the rate at which BlockResponse messages are produced (their rate is what was causing the problem). Still, it doesn't guarantee that the spurious failures won't happen again, it just makes it less likely (hopefully much less).On the other hand,
MessagingService::send_messagebeing async will allow us to use a bounded channel for sync messages in production code too, which may be a good thing, see the Note/TODO inp2p/src/net/default_backend/mod.rs