search

rust-lang / futures-rs

Zero-cost asynchronous programming in Rust
https://rust-lang.github.io/futures-rs/
Apache License 2.0
5.34k stars 616 forks source link

Only return `Poll::Ready` from `Sender::poll_flush` if the channel is empty #2746

Open thomaseizinger opened 1 year ago

thomaseizinger commented 1 year ago

I've found it to be very surprising behaviour that poll_flush on a Sender returns Poll::Ready despite the channel not being empty. This PR attempts to fix this by checking the number of messages in the channel and parking the task if is > 0.

Resolves: https://github.com/rust-lang/futures-rs/issues/2504.

thomaseizinger commented 1 year ago

Miri is detecting a deadlock but I am not quite sure why. Help is much appreciated.

thomaseizinger commented 1 year ago

Okay, I think I found the problem. SinkExt::send implies flush which caused a deadlock if we have a task that suspends on send and doesn't at the same time read from the receiver.

I've replaced that with feed which makes it work again.

This makes me wonder whether this might perhaps be a fairly intrusive change for the ecosystem. I still think that this is the more correct behaviour but I wonder, how many people are relying on SinkExt::send essentially being equivalent to SinkExt::feed.

thomaseizinger commented 1 year ago

Not sure why https://github.com/rust-lang/futures-rs/actions/runs/5046514587/jobs/9052136457?pr=2746#step:5:326 is failing. It passes locally. Help would be appreciated.

taiki-e commented 1 year ago

Thanks for the PR.

Not sure why https://github.com/rust-lang/futures-rs/actions/runs/5046514587/jobs/9052136457?pr=2746#step:5:326 is failing.

armv7 failure is unrelated to this PR (should be fixed by rebase).

This makes me wonder whether this might perhaps be a fairly intrusive change for the ecosystem.

Yeah, I'm not open to introducing a patch that may introduce hard-to-debug deadlocks. (even if it is in a breaking release)

thomaseizinger commented 1 year ago

This makes me wonder whether this might perhaps be a fairly intrusive change for the ecosystem.

Yeah, I'm not open to introducing a patch that may introduce hard-to-debug deadlocks. (even if it is in a breaking release)

To be fair, it only occurs if both sender and receiver are polled by the same task. I think that is a rather unlikely situation because you could also just push and pop a Vec then instead of using a channel.

0x7CFE commented 3 months ago

Any chance this will be merged someday?