-Fixed broadcast storm when entire packet is sent and poll_send_to re…

[Visic]

This PR fixes issue #12

[willstott101]

Thanks, I wonder if we should also have a max number of tries in the incomplete packet branch...

[willstott101]

Would you mind re-basing? It looks like the integration test wasn't configured to run against PRs 🤦

[Visic]

I completely agree about limiting the number of retries, I left it out of this PR so the changes would be simpler to review, I went ahead and opened another issue for it (Issue: #14 ).

I will try to rebase it later today, I'm not able to pull my repo while at work.

[willstott101]

I've pushed to a branch in this repo to get the action to run and it passes fine 👍

But I've done a little digging and found that in 0.2.x (which is still used in librespot) we only try to send once...

        loop {
            if let Some(&(ref response, ref addr)) = self.outgoing.front() {
                trace!("sending packet to {:?}", addr);

                match self.socket.send_to(response, addr) {
                    Ok(_) => (),
                    Err(ref ioerr) if ioerr.kind() == WouldBlock => break,
                    Err(err) => warn!("error sending packet {:?}", err),
                }
            } else {
                break;
            }

            self.outgoing.pop_front();
        }

Looks like the loop was shifted to each packet in @ryanpresciense 's #3 which was picked up by @ashthespy in #4

Unless there's any objection from those two I propose we use while let ...pop_front() and only try to send the packet once.

Other machines polling with mdns will effectively cause a retry periodically, and perhaps that'll be enough?

        while let Some((ref response, ref addr)) = pinned.outgoing.pop_front() {
            trace!("sending packet to {:?}", addr);

            match pinned.socket.poll_send_to(cx, response, addr) {
                Poll::Ready(Ok(bytes_sent)) if bytes_sent == response.len() => (),
                Poll::Ready(Ok(_)) => warn!("failed to send entire packet"),
                Poll::Ready(Err(ref ioerr)) if ioerr.kind() == WouldBlock => (),
                Poll::Ready(Err(err)) => warn!("error sending packet {:?}", err),
                Poll::Pending => (break),
            }
        }

[willstott101]

If we do want to add limited retrys we should do it within the while let ...pop_front() loop.

[Visic]

Personally, I'm happy with it only attempting once as long as it warn!'s when there is a failure, I suspect most clients will be polling anyways. If it turns out to be too flaky on certain networks, it can always be addressed it in a more sophisticated way in the future (retry, exponential backoff, etc.)

[Visic]

What is the next step on this? Did you want me to change over to the implementation you suggested before merging?

[willstott101]

Sorry about the delay on this. I've just pushed a commit I had sitting around for a while: https://github.com/librespot-org/libmdns/tree/broadcast_loop

Would you mind testing that that works for you? I mostly manually test the 0.2 branch by using librespot which isn't new Tokio yet.

[Visic]

@willstott101 That branch is working for me

[willstott101]

Merged and slightly modified in the release of https://github.com/librespot-org/libmdns/releases/tag/v1.4.2