fix(mdns): Don't expire mDNS nodes on connection close #3367
Conversation
felinira
changed the title
felinira
changed the title
There was a problem hiding this comment.
Thanks!
Can we add a test for this? It should be reasonably straight-forward to:
- Boot up two nodes listening on TCP
- Have them discover via mDNS
- Connect to them
- Disconnect them
- Assert that we can still connect to them by only specifying the
PeerId, thus ensuring that the mDNS behaviour still uses the previously discovered record
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
|
I implemented the test only for the async-std backend. Should this be duplicated for tokio or possibly live somewhere else? |
That is fine, thank you :) |
|
The typo helped me realize I somehow committed a working copy of the test instead of the final result. This one should work though. |
| for (peer, addr) in peers { | ||
| if peer == *b.local_peer_id() && !dialed { | ||
| // Connect to all addresses of b to 'expire' all of them | ||
| a.dial(addr)?; | ||
| dialed = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it be better to not iterate and just take the first one? Overriding the boolean state multiple times seems a bit odd.
There was a problem hiding this comment.
The first one might not be b but someone else or even a. But you are right this was a bit convoluted.
|
Ah, now this doesn't work either because it passes the without the MR again. |
|
This works now. This test is a bit tricky because it might happen that the same peer is discovered multiple times and therefore it needs quite a bit of logic to determine the current state to make sure we are not introducing the address again at a later time. |
|
@Mergifyio refresh |
✅ Pull request refreshed |
|
@Mergifyio refresh |
✅ Pull request refreshed |
mergify
bot
dismissed
thomaseizinger’s stale review
Approvals have been dismissed because the PR was updated after the send-it label was applied.
Description
mDNS records should not be expiring when an unrelated connection timeout with said peer is reached.
Fixes #3309.
Links to any relevant issues
#3309
Change checklist