mdns/fix: Failed to register opened substream #301
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
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
skunert
reviewed
Dec 11, 2024
src/protocol/mdns.rs
Outdated
| // Before starting the loop, make an initial query to the network | ||
| if let Err(error) = self.on_outbound_request().await { | ||
| tracing::error!(target: LOG_TARGET, ?error, "Failed to send initial mdns query. MDNS entering failure mode"); | ||
| futures::future::pending::<()>().await; |
There was a problem hiding this comment.
Basically execution would be stuck here forever, right? Is it a problem that self.socket will never be polled then below?
There was a problem hiding this comment.
Would tokio::time::Interval work here? It fires immediately the first time, so there will be no need to send the first outbound request manually.
The caveat is to use Delay MissedTickBehavior, otherwise we might end up bursting many packets if for some reason other branches of select! take too long.
There was a problem hiding this comment.
Yep indeed, have ended up using tokio::time::Interval here 🙏
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
skunert
approved these changes
Dec 12, 2024
dmitry-markin
approved these changes
Dec 12, 2024
lexnv
added a commit
that referenced
this pull request
Dec 12, 2024
## [0.8.4] - 2024-12-12 This release aims to make the MDNS component more robust by fixing a bug that caused the MDNS service to fail to register opened substreams. Additionally, the release includes several improvements to the `identify` protocol, replacing `FuturesUnordered` with `FuturesStream` for better performance. ### Fixed - mdns/fix: Failed to register opened substream ([#301](#301)) ### Changed - identify: Replace FuturesUnordered with FuturesStream ([#302](#302)) - chore: Update hickory-resolver to version 0.24.2 ([#304](#304)) - ci: Ensure cargo-machete is working with rust version from CI ([#303](#303)) cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <[email protected]>
github-merge-queue bot
pushed a commit
to paritytech/polkadot-sdk
that referenced
this pull request
Dec 12, 2024
## [0.8.4] - 2024-12-12 This release aims to make the MDNS component more robust by fixing a bug that caused the MDNS service to fail to register opened substreams. Additionally, the release includes several improvements to the `identify` protocol, replacing `FuturesUnordered` with `FuturesStream` for better performance. ### Fixed - mdns/fix: Failed to register opened substream ([#301](paritytech/litep2p#301)) ### Changed - identify: Replace FuturesUnordered with FuturesStream ([#302](paritytech/litep2p#302)) - chore: Update hickory-resolver to version 0.24.2 ([#304](paritytech/litep2p#304)) - ci: Ensure cargo-machete is working with rust version from CI ([#303](paritytech/litep2p#303)) cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <[email protected]>
lexnv
added a commit
to paritytech/polkadot-sdk
that referenced
this pull request
Dec 12, 2024
## [0.8.4] - 2024-12-12 This release aims to make the MDNS component more robust by fixing a bug that caused the MDNS service to fail to register opened substreams. Additionally, the release includes several improvements to the `identify` protocol, replacing `FuturesUnordered` with `FuturesStream` for better performance. ### Fixed - mdns/fix: Failed to register opened substream ([#301](paritytech/litep2p#301)) ### Changed - identify: Replace FuturesUnordered with FuturesStream ([#302](paritytech/litep2p#302)) - chore: Update hickory-resolver to version 0.24.2 ([#304](paritytech/litep2p#304)) - ci: Ensure cargo-machete is working with rust version from CI ([#303](paritytech/litep2p#303)) cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <[email protected]>
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.
This PR ensures that when MDNS encounters an error it does not terminate other litep2p components.
Previously, if MDNS failed to send a query or to handle the incoming packets it would exit.
The exit is presented by the following log line observed on kusama validator:
This situation is causing the substrate Discovery mechanism to also exit, which propagates to the litep2p kademlia handler that exits as well. This leaves the node unable to discover the network or handle incoming substreams.
Testing Done
The issue was reproduced locally with a tokio interval patch that exits the MDNS component after having connectivity in Kusama:
Closes: #300
Thanks @dmitry-markin for also confirming this 🙏
cc @paritytech/networking