fix(iroh-p2p): ensure providers stream closes
#625
Merged
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
ramfox
force-pushed
the
ramfox/p2p_test
branch
from
e45c88d to
df7ed3e
Compare
ramfox
force-pushed
the
ramfox/p2p_test
branch
3 times, most recently
from
4c421b5 to
3d8a6d0
Compare
dignifiedquire
previously approved these changes
Dec 20, 2022
ramfox
force-pushed
the
ramfox/p2p_test
branch
from
3d8a6d0 to
17f9523
Compare
dignifiedquire
approved these changes
Dec 20, 2022
ppodolsky
pushed a commit
to izihawa/iroh
that referenced
this pull request
Dec 20, 2022
* fix flakey `test_dht` test by extending timeout * add test for `CancelListenForIdentify`
ppodolsky
pushed a commit
to izihawa/iroh
that referenced
this pull request
Dec 20, 2022
* fix flakey `test_dht` test by extending timeout * add test for `CancelListenForIdentify`
ppodolsky
pushed a commit
to izihawa/iroh
that referenced
this pull request
Dec 28, 2022
* fix flakey `test_dht` test by extending timeout * add test for `CancelListenForIdentify`
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.
Also implements final p2p unit test to close n0-computer/beetle#99
Bug fix fixes the flakey dht tests n0-computer/beetle#74
Some notes:
Before we could get to fixing the flakiness, the test was hanging due to a bug in the
Providersquery manager. To summarize the issue: we weren't reacting toGetProvidersOk::FinishedWithNoAdditionalRecordevents, which indicate no more records were found. Because this event was never handled, we never removed theQueryfrom theprovidersSet, and so the stream of providers never closed, causing thetest_dhttest to hang.To test reliability, I looped the
test_dht10 times for my initial commits, but removed the loop it once things stabilized.I did a whole pass where the node emitted an event whenever the kbuckets were updated, and used that event to ensure everything was "in place" before attempting to
StartProvidingandfetch_providers_dht. However... this made things less reliable! So I removed it. It also would have potentially added a lot of "noise" to thenetwork_eventstream.The only thing that actually stabilized the test (when attempting multiple loops)... was increasing the timeout by 1 second. I'm not sure how to feel about this.
In order to test the
CancelListenForIdentify, I had to add aNetworkEventthat would emit after this cancellation happens. There was no other way I could think of that would allow us to inspect the node itself without breaking the borrow checker. This event would only ever be emitted due to a user action (manually attempting to lookup a peer), so I'm not concerned about it polluting the event space.