Channel refactor #985
Channel refactor #985
Conversation
| @@ -401,7 +401,8 @@ fn stress_close_receiver_iter() { | |||
| Some(r) => assert!(i == r), | |||
| None => { | |||
| let unwritten = unwritten_rx.recv().expect("unwritten_rx"); | |||
| assert_eq!(unwritten, i); | |||
| assert!(unwritten <= i + 1); | |||
There was a problem hiding this comment.
This is necessary in order to prevent this test from flaking due to the race condition when writing into a closed channel.
There was a problem hiding this comment.
I assume that this is the race that you mentioned in gitter and you are punting on a fix? If so, could you add a TODO referencing the issue?
There was a problem hiding this comment.
Skimming over it looks like the unbounded variant also has the same issue?
There was a problem hiding this comment.
Yes, they both have this issue.
|
Could you explain the reasoning behind splitting up the unbounded & bounded implementations? |
|
It's a pretty large diff and hard for me to reason about due to how terminology changes are mixed with code and behavior changes. |
Sure! I thought it was clearer and easier to read with them split up this way, and seemed like it might be more performant since there are fewer queues, atomics, and other checks needed to supply simple receiver wakeups than the comparatively complex sender wakeup semantics. |
cramertj
force-pushed
the
channel-backpressure
branch
from
8fcfc9c
to
822061b
Compare
|
Before this PR: After this PR: |
|
The main argument against duplicating code is that it introduces more locations for bugs. For example, the close race bug is now duplicated in two code locations. I would suggest that, if performance is the driver, at the very least benchmarks illustrating the performance difference should be present and probably at least a basic tuning pass of the bounded mpsc should be done (given that none have been done to date). |
|
At the end of the day, you are the ones who have to maintain it, so feel free to do whatever. |
futures-channel/src/mpsc/mod.rs
Outdated
| } | ||
| }; | ||
| // Return the message | ||
| // Try to read a message off of the message queue. |
There was a problem hiding this comment.
Seems worth factoring this out into its own method, since the code is repeated verbatim below.
cramertj
force-pushed
the
channel-backpressure
branch
from
822061b
to
4d2509f
Compare
Refactors channels to use "waker" terminology rather than "parking" Splits unbounded channels into a separate, simplified underlying type Calls poll_ready in poll_flush impl for bounded channels in order to provide backpressure on <Sender as Sink>::send.
cramertj
force-pushed
the
channel-backpressure
branch
from
4d2509f
to
c542a62
Compare
Refactors channels to use "waker" terminology rather than "parking"
Splits unbounded channels into a separate, simplified underlying type
Calls poll_ready in poll_flush impl for bounded channels in order to
provide backpressure on ::send.
Replacement for #984.
Fix #800
Fix rustasync/team#11
cc @danburkert, @seanmonstar, @carllerche