misc/mdns: Update to futures-preview #1247

mxinden · 2019-09-18T13:16:14Z

This pull request updates misc/mdns to use futures-preview 0.3.0-alpha.17. It is ready for review but still has a couple of TODO comments that I would like to have your input on.

This is an optimization only important for short lived use cases, e.g. unit tests. Instead of waiting for 20 seconds at first, the query interval fires right away and thereby the service makes progress instantly.

tomaka

I think we might have to ask for help from the async-std people.

tomaka · 2019-09-18T15:39:12Z

misc/mdns/src/service.rs

@@ -147,21 +158,27 @@ impl MdnsService {
        Ok(MdnsService {
            socket,
            query_socket: UdpSocket::bind((Ipv4Addr::from([0u8, 0, 0, 0]), 0u16)).await?,
-            query_interval: Interval::new(Duration::from_secs(20)),
+            query_interval: Interval::new_at(Instant::now(), Duration::from_secs(20)),


Why change this?

Simply to not have to wait for 20 seconds within the unit tests. Other than adding a bit of complexity I don't see it having an impact at the compiled logic.

The same is done in current master:

rust-libp2p/misc/mdns/src/service.rs

Line 161 in 46c62b0

query_interval: Interval::new_at(Instant::now(), Duration::from_secs(20)),

tomaka · 2019-09-18T15:40:44Z

misc/mdns/src/service.rs

+            // around this with the additional scope on match. Is there a
+            // prettier way?
+            match {
+                let fut = self.query_socket.send_to(&to_send, *IPV4_MDNS_MULTICAST_ADDRESS);


That doesn't work, you're dropping the future. There's a lot more code to write here for it to work unfortunately.

tomaka · 2019-09-18T15:43:04Z

misc/mdns/src/service.rs

+        match {
+            let fut = self.socket.recv_from(&mut self.recv_buffer);
+            futures::pin_mut!(fut);
+            fut.poll(cx)


Same remark.

…-mdns

Libp2p-mdns uses the async-std crate for network io. This crate only offers async send and receive functions. In order to use this in non async/await functions one needs to keep the future returned by the crate functions around across `poll` invocations. The future returned by the crate functions references the io resource. Thus one has to keep both the io resource as well as the future referencing it. This results in a self-referencing struct which is not possible to create with safe Rust. Instead, by having `MdnsService::next` (former `MdnsService::poll`) take ownership of `self`, the Rust async magic takes care of the above (See code comments for more details). As a (negative) side effect, given that `MdnsService::next` takes ownership of `self`, there is nothing to bind the lifetime of the returned `MdnsPacket` to. With no better solution in mind, this patch makes `MdnsPacket` static, not referencing the `MdnsService` receive buffer.

mxinden · 2019-10-30T15:42:14Z

As discussed I included option 2 (not dropping the udp socket futures by having poll (now next) take ownership of itself). See 40a043e.

@tomaka @twittner when you have some time, could you give this another review?

tomaka · 2019-11-06T08:50:47Z

misc/mdns/src/behaviour.rs


    /// List of nodes that we have discovered, the address, and when their TTL expires.
    ///
    /// Each combination of `PeerId` and `Multiaddr` can only appear once, but the same `PeerId`
    /// can appear multiple times.
+    //
+    // TODO: Why this optimization? Is mdns ever within the hot path of an


This isn't an optimization. It is necessary in order to store the Multiaddresses of the nodes we know about. Without it, you can't implement addresses_of_peer.

Why can this not be a simple heap allocated Vec instead of a maybe stack allocated SmallVec?

I guess it doesn't really matter.

tomaka · 2019-11-06T08:51:45Z

misc/mdns/src/behaviour.rs

    ///
    /// `None` if `discovered_nodes` is empty.
+    // TODO: Would a simple std Instant suffice as well?


We currently report expired nodes as events of the behaviour. If we want these to be reported "pro-actively", then it has to be a Delay.

Makes sense. Thanks!

tomaka

In general, the API of the service doesn't really make sense anymore now that the MdnsPacket is detached from the service.

tomaka · 2019-11-06T08:52:34Z

misc/mdns/src/behaviour.rs

    closest_expiration: Option<Delay>,

    /// Marker to pin the generic.
    marker: PhantomData<TSubstream>,
 }

+/// Polling the async MdnsService within a non-async `poll` function forces one to keep the returned


I don't think you need to explain that. The second part of the doc-comment is enough.

tomaka · 2019-11-06T08:53:06Z

misc/mdns/src/behaviour.rs

-            let event = match self.service.poll(cx) {
-                Poll::Ready(ev) => ev,
-                Poll::Pending => return Poll::Pending,
+            let service = std::mem::replace(&mut self.service, MaybeBusyMdnsService::Unreachable);


Style nit-pick.

Suggested change

let service = std::mem::replace(&mut self.service, MaybeBusyMdnsService::Unreachable);

let service = mem::replace(&mut self.service, MaybeBusyMdnsService::Unreachable);

tomaka · 2019-11-06T08:57:08Z

misc/mdns/src/behaviour.rs

+enum MaybeBusyMdnsService {
+    Free(MdnsService),
+    Busy(Pin<Box<dyn Future<Output = (MdnsService, MdnsPacket)> + Send>>),
+    Unreachable,


This is technically reachable if we have a panic and unwind, so I'd prefer if this was called Poisoned.

Suggested change

Unreachable,

Poisoned,

tomaka · 2019-11-06T08:57:55Z

misc/mdns/src/behaviour.rs

+                            // TODO: This should move to a constant, right?
+                            Duration::from_secs(5 * 60)
+                        ).unwrap());
+                    }


(something like that)

Suggested change

}

} else { debug_assert!(false); }

tomaka · 2019-11-06T09:00:07Z

misc/mdns/src/behaviour.rs

+                        // TODO: This should move to a constant, right? Also can it be consolidated with
+                        // the query response TTL?
+                        service.enqueue_response(disc.build_response(Duration::from_secs(5 * 60)));
+                    }


Suggested change

}

} else { debug_assert!(false); }

tomaka · 2019-11-06T09:05:38Z

misc/mdns/src/service.rs

+
+    /// Returns a future resolving to itself and the next received `MdnsPacket`.
+    ///
+    /// **Note**: Why does `next` take ownership of itself?


I would explain this in a regular comment, but not in a doc-comment.

tomaka · 2019-11-06T09:07:32Z

misc/mdns/src/service.rs

-                    let query = dns::build_query();
-                    self.query_send_buffers.push(query.to_vec());
-                }
+        if let Some(_) = self.query_interval.next().now_or_never() {


That's not correct, unfortunately.
Once you reach recv().await, you will block forever until the socket has data for you.
Instead, what we want is for query_interval to "interrupt" that and dispatch a new request.

tomaka · 2019-11-06T09:11:05Z

misc/mdns/src/service.rs

        self,
        peer_id: PeerId,
        addresses: TAddresses,
        ttl: Duration,
-    ) -> Result<(), MdnsResponseError>
+    ) -> Result<Vec<u8>, MdnsResponseError>


Well, that's not great.
The respond method was a way to tell the service to send a response.
Now that MdnsQuery is detached from the service, it doesn't make sense to keep the API similar to what it was before.

Can you add some more details here? Due to the fact that MdnsQuery does not have a reference to the send_buffer anymore it only builds the response, which later on is send by the Mdns NetworkBehaviour implementation to the MdnsService.

What I mean is that it doesn't make sense for the MdnsQuery object itself to build the response. With the exception of query_id, the self parameter isn't necessary. It used to be like that because self.send_buffers is an implementation detail that is hidden from the user.
This method should now be a free-standing function instead.

tomaka · 2019-11-06T09:11:37Z

misc/mdns/src/service.rs

-    pub fn respond(self, ttl: Duration) {
-        let response = dns::build_service_discovery_response(self.query_id, ttl);
-        self.send_buffers.push(response);
+    pub fn build_response(self, ttl: Duration) -> Vec<u8> {


Same remark.

tomaka · 2019-11-06T09:12:00Z

misc/mdns/src/service.rs

-        self.packet.answers.iter().filter_map(move |record| {
+impl MdnsResponse {
+    /// Creates a new `MdnsResponse` based on the provided `Packet`.
+    pub fn new(packet: Packet, from: SocketAddr) -> MdnsResponse {


Suggested change

pub fn new(packet: Packet, from: SocketAddr) -> MdnsResponse {

fn new(packet: Packet, from: SocketAddr) -> MdnsResponse {

…-mdns

When we 'await' on receiving a packet on the udp socket without receiving a single packet we starve the remaining logic of the mdns service, in this case the logic triggered on the receive interval.

mxinden · 2019-11-11T18:55:38Z

@tomaka Could you take another look at this?

tomaka

The general polling logic looks good 👍 , but the API surface not so much in my opinion.
I'll take another look and might have more comments.

tomaka · 2019-11-11T21:13:49Z

misc/mdns/src/service.rs

+    /// Returns the list of peers that have been reported in this packet.
+    ///
+    /// > **Note**: Keep in mind that this will also contain the responses we sent ourselves.
+    pub fn discovered_peers(&self) -> &Vec<MdnsPeer> {


Suggested change

pub fn discovered_peers(&self) -> &Vec<MdnsPeer> {

pub fn discovered_peers(&self) -> impl Iterator<Item = &MdnsPeer> {

tomaka · 2019-11-11T21:16:13Z

misc/mdns/src/service.rs

-        self.packet
+impl MdnsPeer {
+    /// Creates a new `MdnsPeer` based on the provided `Packet`.
+    pub fn new(packet: &Packet, record_value: String, my_peer_id: PeerId, ttl: u32) -> MdnsPeer {


MdnsPeer is supposed to be a borrow for a value inside of a Packet, similar to a MutexGuard being a borrow of a Mutex for example.

Packet is not static, thus we need to do the parsing when constructing the MdnsPeer as the final MdnsPeer needs to be static.

tomaka · 2019-11-11T21:16:32Z

misc/mdns/src/service.rs

    use crate::service::{MdnsPacket, MdnsService};
    use multiaddr::multihash::*;

    fn discover(peer_id: PeerId) {
-        let mut service = MdnsService::new().unwrap();
-        let stream = stream::poll_fn(move |cx| -> Poll<Option<Result<(), io::Error>>> {
+        block_on (async {


Suggested change

block_on (async {

block_on(async {

tomaka · 2019-11-11T21:16:57Z

misc/mdns/src/service.rs

                            }
                        }
                    }
-                    MdnsPacket::ServiceDiscovery(_) => {}
+                    MdnsPacket::ServiceDiscovery(_) => {panic!("did not expect a service discovery packet")}


Suggested change

MdnsPacket::ServiceDiscovery(_) => {panic!("did not expect a service discovery packet")}

MdnsPacket::ServiceDiscovery(_) => panic!("did not expect a service discovery packet")

tomaka · 2019-11-11T21:17:05Z

misc/mdns/src/service.rs

+        })
+    }
+
+    #[test]


Suggested change

#[test]

tomaka · 2019-11-11T21:17:17Z

misc/mdns/src/service.rs

+    #[test]
+    // As of today the underlying UDP socket is not stubbed out. Thus tests run in parallel to this
+    // unit tests inter fear with it. Test needs to be run in sequence to ensure test properties.
+    fn respect_query_interval() {


Suggested change

fn respect_query_interval() {

#[test]

fn respect_query_interval() {

mxinden · 2019-11-11T22:12:23Z

Thanks for bearing with me @tomaka. The above comment and 17fd131 should address your feedback.

tomaka

Sorry, I forgot about this PR.
We had some IRL chat last week about that, and I think it's fine! So, let's merge this if no one has an objection.

mxinden added 10 commits September 18, 2019 15:05

misc/mdns/service: Use async std with stack pinned futures

6db81af

misc/mdns: Define mdns broadcast address as lazy static

d28fecd

misc/mdns: Drop future before borrowing their arguments again

fcf8e8c

misc/mdns: Send queries on query socket, not socket

7e86c66

misc/mdns: Use poll_next_unpin on query interval stream

ccf547e

misc/mdns: Ensure underlying task is woken up on next interval tick

c3e6377

misc/mdns: Wrap match expression in scope to drop future early

2ffdc4f

misc/mdns: Adjust 'discovery_ourselves' test

9daec0d

misc/mdns: Make query interval fire instantly on first tick

bfaee2f

This is an optimization only important for short lived use cases, e.g. unit tests. Instead of waiting for 20 seconds at first, the query interval fires right away and thereby the service makes progress instantly.

misc/mdns: Adjust MdnsService documentation tests

46c62b0

tomaka suggested changes Sep 18, 2019

View reviewed changes

mxinden added 3 commits October 26, 2019 10:34

Merge remote-tracking branch 'libp2p/stable-futures' into new-futures…

b7fada7

…-mdns

misc/mdns: Fix code comments and remove *if Free* TODO

eb5b06a

tomaka reviewed Nov 6, 2019

View reviewed changes

tomaka suggested changes Nov 6, 2019

View reviewed changes

mxinden added 11 commits November 8, 2019 22:06

misc/mdns: Minor refactorings

7f28883

misc/mdns: Build responses in behaviour.rs directly

09c60f1

misc/mdns: Move response ttl duration to constant

5181f64

misc/mdns: Remove optimization todo comment

e037478

misc/mdns: Add query interval test

d8d04f6

Merge remote-tracking branch 'libp2p/stable-futures' into new-futures…

5b678b7

…-mdns

misc/mdns: Move packet parsing into MdnPacket impl

ca8c95e

misc/mdns: Don't have receiving packets starve the query interval

999bcb5

When we 'await' on receiving a packet on the udp socket without receiving a single packet we starve the remaining logic of the mdns service, in this case the logic triggered on the receive interval.

misc/mdns: Add debug_assert to MaybeBusyMdnsService check

1632d3f

misc/mdns: Implement Debug for MaybeBusyMdnsService

0a2696b

misc/mdns: Make ownership note a normal comment, not a doc comment

1dfc0ff

mxinden marked this pull request as ready for review November 11, 2019 18:55

tomaka suggested changes Nov 11, 2019

View reviewed changes

misc/mdns: Have discovered_peers return an iterator

17fd131

tomaka approved these changes Nov 19, 2019

View reviewed changes

tomaka merged commit a26620b into libp2p:stable-futures Nov 20, 2019

mxinden mentioned this pull request Jan 6, 2020

Refactor the crate to use stable futures #1328

Merged

mxinden mentioned this pull request Mar 10, 2020

QUIC support #1334

Closed

romanb mentioned this pull request Dec 3, 2020

mDNS shouldn't panic if response packet becomes too large #1872

Closed

	let service = std::mem::replace(&mut self.service, MaybeBusyMdnsService::Unreachable);
	let service = mem::replace(&mut self.service, MaybeBusyMdnsService::Unreachable);

	pub fn new(packet: Packet, from: SocketAddr) -> MdnsResponse {
	fn new(packet: Packet, from: SocketAddr) -> MdnsResponse {

	pub fn discovered_peers(&self) -> &Vec<MdnsPeer> {
	pub fn discovered_peers(&self) -> impl Iterator<Item = &MdnsPeer> {

	MdnsPacket::ServiceDiscovery(_) => {panic!("did not expect a service discovery packet")}
	MdnsPacket::ServiceDiscovery(_) => panic!("did not expect a service discovery packet")

+                      })
+                  }
+                  #[test]

	fn respect_query_interval() {
	#[test]
	fn respect_query_interval() {

misc/mdns: Update to futures-preview #1247

misc/mdns: Update to futures-preview #1247

Conversation

mxinden commented Sep 18, 2019

tomaka left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka Nov 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Nov 11, 2019

tomaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Nov 11, 2019

tomaka left a comment

Choose a reason for hiding this comment

tomaka left a comment •

edited

Loading

tomaka Nov 9, 2019 •

edited

Loading