keepsimple1 / mdns-sd

Rust library for mDNS based Service Discovery
Apache License 2.0
89 stars 38 forks source link

When a receiver is dropped, the error produced by sender should not be Error #173

Closed irvingoujAtDevolution closed 4 months ago

irvingoujAtDevolution commented 4 months ago

It is very possible that mDNS scan is being done in an async manner with timeout, when a future that containing the receiver is dropped when timed out, we naturely wants to stop brose using the stopBrowse method to clean up resources.

keepsimple1 commented 4 months ago

This library does not use async, can you please elaborate the details? And what do you mean by the receiver and sender here?

irvingoujAtDevolution commented 4 months ago

the receiver refers to the flume receiver when browsing a new service

let receiver = service_daemon.browse(service_name.as_ref()).with_context(|| {
            let err_msg = format!("failed to browse for service: {}", service_name);
            error!(error = err_msg);
            err_msg
})?;

The sender of course, refers to the sender on the other side of the channel.

Flume supports async send and with the receiver trait

impl<T> Receiver<T> {
    /// Asynchronously receive a value from the channel, returning an error if all senders have been
    /// dropped. If the channel is empty, the returned future will yield to the async runtime.
    pub fn recv_async(&self) -> RecvFut<'_, T> { <<-------------- This is returns a future
        RecvFut::new(OwnedOrRef::Ref(self))
    }

   //....
}

and commonly, one may want to use tokio timeout for a maximum wait time. if timed out, we usually stop browsing, and the sender will fail to send an event as the tokio timeout drops the future after timed out.

keepsimple1 commented 4 months ago

I see. The error refers to this one, right?

I assume that in your code it is not easy, or doesn't make much sense to read SearchStopped event, right? As you already are in the process of drop after a time out.

As this is not critical error, we could reduce the logging level of this one to WARN or INFO. Is WARN okay or no?

irvingoujAtDevolution commented 4 months ago

Yes! INFO is preferred on my side, but for the sake of the project, I think WARN might be the better choice. At whichever you prefer

keepsimple1 commented 4 months ago

Fix merged. Let me know if any problems.