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

misc/mdns: Make MdnsService::new sync by using std::net::UdpSocket::bind #1382

Merged
merged 6 commits into from
Jan 15, 2020

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Jan 9, 2020

MdnsService uses an IP address to create a UDP socket. The address does
not need to be resolved. Therefore one can use std's UdpSocket::bind
instead of the async counterpart from async-std. As a result
MdnsService::new and MdnsService::silent don't need to be async.

#1328 (comment)

(//CC @demimarie-parity)

MdnsService uses an IP address to create a UDP socket. The address does
not need to be resolved. Therefore one can use std's UdpSocket::bind
instead of the async counterpart from async-std. As a result
MdnsService::new and MdnsService::silent don't need to be async.
@@ -90,7 +90,7 @@ impl<TSubstream> Mdns<TSubstream> {
/// Builds a new `Mdns` behaviour.
pub async fn new() -> io::Result<Mdns<TSubstream>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub async fn new() -> io::Result<Mdns<TSubstream>> {
pub fn new() -> io::Result<Mdns<TSubstream>> {

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. That makes a lot of sense. Thanks.

@Demi-Marie
Copy link
Contributor

Looks good to me, other than @twittner's correction.

@mxinden
Copy link
Member Author

mxinden commented Jan 10, 2020

Should we merge this after we released v0.14.0, given that it is a breaking change to the API surface?

@tomaka
Copy link
Member

tomaka commented Jan 13, 2020

Should we merge this after we released v0.14.0, given that it is a breaking change to the API surface?

We've already got a few breaking changes in, so let's go ahead with that.

@tomaka tomaka merged commit 991f5af into libp2p:master Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants