Potential blocking issue when use tracing_unbounded channel to send Sender<T>

[NingLin-P]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Description of bug

In #13504, the underlying channel of tracing_unbounded channel switched from futures-channel to async-channel.

In async-channel, the unconsumed messages of the channel will not drop even though all the receivers have been dropped and these messages can never be accessed smol-rs/async-channel#23, this behavior may be an issue when using tracing_unbounded channel to send Sender<T> to another worker and wait for receiving from this Sender<T> (like the usage mentioned at smol-rs/async-channel#23).

In subspace/subspace#1352 (comment), we have encountered an issue with the abovementioned usage which causes our test block forever. I'm not quite familiar with the tracing_unbounded usage in substrate, but I do observe some use cases that use tracing_unbounded channel to send Sender<T> and they may be potential issues:

substrate/client/utils/src/status_sinks.rs

Line 61 in 851a826

let (entries_tx, entries_rx) = tracing_unbounded("status-sinks-entries", 100_000);

substrate/client/peerset/src/lib.rs

Line 278 in 851a826

let (tx, rx) = tracing_unbounded("mpsc_peerset_messages", 10_000);

substrate/client/network/sync/src/service/network.rs

Line 124 in 851a826

let (tx, rx) = tracing_unbounded("mpsc_network_service_provider", 100_000);

substrate/client/network/sync/src/engine.rs

Line 331 in 851a826

let (tx, service_rx) = tracing_unbounded("mpsc_chain_sync", 100_000);

Steps to reproduce

See smol-rs/async-channel#23

[bkchr]

I mean a proper solution would be to fix upstream, otherwise we could just switch the internal implementation to something different that doesn't has this issue.

[NingLin-P]

I mean a proper solution would be to fix upstream

Agree, another option is to switch back to futures-channel which seems does not have this behavior and there is actually a unit test to cover this case: https://github.com/rust-lang/futures-rs/blob/5ac72c751c406a525416ad56ead4009a5994b613/futures-channel/tests/mpsc-close.rs#L107

[bkchr]

Would you be open to provide a pr switching the internal implementation @NingLin-P?

[bkchr]

Ahh, we can not switch to futures-channel. We just recently switched to async-channel to support checking the length of the channel. I mean you also linked the pr :D

[NingLin-P]

Right... Or how about we manually implement that in tracing_unbounded, to drain all the pending messages in the async-channel channel when TracingUnboundedReceiver is dropped? I'm happy to contribute if you think this approach is feasible.

[bkchr]

If we first close the receiver and then drain all the messages, it should work. Sounds like something to try out. I'm happy if you could do this @NingLin-P :)

[NingLin-P]

Sure! Let me try.