Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(mdns): adaptive initial interval peer discovery #3975

Merged
merged 7 commits into from
May 25, 2023

Conversation

stormshield-pj50
Copy link
Contributor

@stormshield-pj50 stormshield-pj50 commented May 22, 2023

Description

Peer discovery with mDNS can be very slow, particularly if the first mDNS query is lost. This patch resolves it by adjusting the timer with an adaptive initial interval. We start with a very short timer (500 ms). If a peer is discovered before the end of the timer, then the timer is reset to the normal query interval value (300s), otherwise the timer's value is multiplied by 2 until it reaches the normal query interval value.

Related: #3323.
Resolves: #3319.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@stormshield-pj50 stormshield-pj50 changed the title feat(mdns): add a neighbour discovery logic using an adaptive initial interval feat(mdns): adaptive initial interval peer discovery May 22, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Two comments. Let me know what you think.

protocols/mdns/src/behaviour/iface.rs Outdated Show resolved Hide resolved
protocols/mdns/src/behaviour/iface.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One more question

@@ -159,6 +186,25 @@ where
if Pin::new(&mut self.timeout).poll_next(cx).is_ready() {
log::trace!("sending query on iface {}", self.addr);
self.send_buffer.push_back(build_query());
log::trace!("tick on {:#?} {:#?}", self.addr, self.probe_state);

let continue_probing = self.discovered.is_empty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this here if we set it on line 268 already?

I think it is easier to understand if we pair the changes to probe_state with the changes to discovered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right the check on self.discovered when the timer triggers is useless

@stormshield-pj50 stormshield-pj50 force-pushed the feat_mdns_initial_delay branch from 84e72c3 to 1366ea2 Compare May 24, 2023 07:57
thomaseizinger
thomaseizinger previously approved these changes May 24, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thank you!

I wonder if we should have an automated test for this? It is a bit tricky. If you have an idea, it would be much appreciated.

Can you confirm that the current version works as expected?

@thomaseizinger
Copy link
Contributor

Clippy doesn't seem to be happy though.

@stormshield-pj50
Copy link
Contributor Author

This looks good to me, thank you!

I wonder if we should have an automated test for this? It is a bit tricky. If you have an idea, it would be much appreciated.

Can you confirm that the current version works as expected?

On our side we have an automated docker testsuite and the first mDNS query can be lost if the interface is not ready yet.

Anyway the current version works, we just did plug our manual libp2p test tool with it and the mdns peer discovery works as expected, both with a successful and an unsuccessful discovery, the probe ticks logs are OK.

@thomaseizinger
Copy link
Contributor

This looks good to me, thank you!
I wonder if we should have an automated test for this? It is a bit tricky. If you have an idea, it would be much appreciated.
Can you confirm that the current version works as expected?

On our side we have an automated docker testsuite and the first mDNS query can be lost if the interface is not ready yet.

Anyway the current version works, we just did plug our manual libp2p test tool with it and the mdns peer discovery works as expected, both with a successful and an unsuccessful discovery, the probe ticks logs are OK.

The code looks good to me too. I am fine merging without an automated test. Thanks for confirming!

mxinden
mxinden previously approved these changes May 25, 2023
protocols/mdns/CHANGELOG.md Show resolved Hide resolved
@mergify mergify bot dismissed stale reviews from thomaseizinger and mxinden May 25, 2023 03:50

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit 67b26cc into libp2p:master May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manage an initial delay before the first mdns query is sent on an interface
3 participants