Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 4 commits into from

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented May 22, 2023 •
edited

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: #2504.

@thomaseizinger thomaseizinger changed the title Only return Poll::Ready from Sender if the channel is empty Only return Poll::Ready from Sender::poll_flush if the channel is empty May 22, 2023
@thomaseizinger thomaseizinger mentioned this pull request May 22, 2023
Merged
@thomaseizinger
Copy link
Contributor Author

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

@thomaseizinger
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

mxinden pushed a commit to libp2p/rust-yamux that referenced this pull request May 24, 2023
@thomaseizinger
fix: avoid race condition between pending frames and closing stream (#…
52c725b 
…156)

Currently, we have a `garbage_collect` function that checks whether any of our
streams have been dropped. This can cause a race condition where the channel
between a `Stream` and the `Connection` still has pending frames for a stream
but dropping a stream causes us to already send a `FIN` flag for the stream.

We fix this by maintaining a single channel for each stream. When a stream gets
dropped, the `Receiver` becomes disconnected. We use this information to queue
the correct frame (`FIN` vs `RST`) into the buffer. At this point, all previous
frames have already been processed and the race condition is thus not present.

Additionally, this also allows us to implement `Stream::poll_flush` by
forwarding to the underlying `Sender`. Note that at present day, this only
checks whether there is _space_ in the channel, not whether the items have been
emitted by the `Receiver`.

We have a PR upstream that might fix this:
rust-lang/futures-rs#2746

Fixes: #117.
@taiki-e taiki-e added A-channel Area: futures::channel S-needs-decision Status: A decision on whether or not to do this is needed. labels Jul 19, 2023
@taiki-e
Copy link
Member

taiki-e commented Jul 19, 2023 •
edited

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)

@taiki-e taiki-e removed the S-needs-decision Status: A decision on whether or not to do this is needed. label Jul 19, 2023
@thomaseizinger
Copy link
Contributor Author

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.

@thomaseizinger thomaseizinger mentioned this pull request Jan 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-channel Area: futures::channel
Projects
None yet
Milestone
No milestone
2 participants
@thomaseizinger @taiki-e