fix: spawn tasks in Stream::buffered and Stream::buffer_unordered to max concurrency #2962
Conversation
rustbot
added
A-stream
rustbot
removed
the
S-waiting-on-review
rustbot
added
the
S-waiting-on-review
| @@ -13,20 +14,23 @@ pin_project! { | |||
| /// Stream for the [`buffer_unordered`](super::StreamExt::buffer_unordered) | |||
| /// method. | |||
| #[must_use = "streams do nothing unless polled"] | |||
| pub struct BufferUnordered<St> | |||
| pub struct BufferUnordered<St, F> | |||
There was a problem hiding this comment.
Can we fix this in FuturesUnordered? This could actually be a breaking change.
There was a problem hiding this comment.
Can we fix this in
FuturesUnordered?
I've tried to implement it in andylokandy@1d9fe21.
The short answer is no -- many components in futures -rs relies on the feature of FuturesUnordered not greedily poll internal futures. Instead, buffered is supposed to provide the ability to maximize the concurrency respecting the limit.
| x @ Poll::Pending | x @ Poll::Ready(Some(_)) => return x, | ||
| Poll::Ready(None) => {} | ||
| // Try to poll all ready futures in the in_progress_queue. | ||
| loop { |
There was a problem hiding this comment.
I believe this fix could be incorrect, as it assumes that tasks are finished in order. And it introduces extra costs.
I have similar issues in opendal, and my final solution is to find a way to track the status of futures and subtract the already finished ones from the maximum concurrent limit.
Do you think it's a good idea to implement similiar thing in FuturesUnordered?
There was a problem hiding this comment.
I believe this fix could be incorrect, as it assumes that tasks are finished in order.
Would you like to elaborate more why you think it assumes that tasks are finished in order?
| pub struct Buffered<St, F> | ||
| where | ||
| St: Stream, | ||
| St::Item: Future, | ||
| St: Stream<Item = F>, | ||
| F: Future, |
There was a problem hiding this comment.
Seems like a breaking change?
There was a problem hiding this comment.
It seems ok since we are on 0.4-alpha
Closes #2961