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.

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

Avoid starvation from FuturesUnordered::poll_next #2049

Merged
merged 1 commit into from Jan 27, 2020

Conversation

@jonhoo
Copy link
Contributor

jonhoo commented Jan 23, 2020

This fixes #2047.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 23, 2020

I'm unsure about this. Overall, I think we need some better way to address this issue globally, as this behavior is also an issue with every other kind of Future/Stream/asynchronous function. If a task has work remaining that can be done, it will never yield to the executor. Making every single looping combinator track a maximum poll count seems like a possibility, but it seems like the kind of practice we'd want some discipline around, rather than picking arbitrary integers which will stack exponentially (with this PR, the limit of leaf-future polls for a FuturesUnordered containing futures which poll a FuturesUnordered is now 32^2=1024, and another layer would make it 32768). Perhaps @carllerche has thoughts?

@jonhoo

This comment has been minimized.

Copy link
Contributor Author

jonhoo commented Jan 23, 2020

Yup, I agree it's not ideal, which is why I also proposed tokio-rs/tokio#2160 as a more "complete" fix. I actually argued the exact same point about exponentials in the tokio Discord a few hours ago as a reason why we ultimately want a more global mechanism :p I do think we need a solution in the short run too though, as global preemption is probably something that will take longer to settle, and this is certainly an issue right now (for example, I am running into it in Noria as we speak).

@carllerche

This comment has been minimized.

Copy link
Member

carllerche commented Jan 23, 2020

I agree w/ @cramertj. IMO this should be tracked at the runtime task level and leaf resource operations should be the ones to impact the poll budget.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 27, 2020

Merging-- see discussion ongoing in #2047

@cramertj cramertj merged commit e767803 into rust-lang:master Jan 27, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@jonhoo jonhoo deleted the jonhoo:yielding-futuresunordered branch Jan 28, 2020
@jonhoo

This comment has been minimized.

Copy link
Contributor Author

jonhoo commented Jan 28, 2020

@cramertj What's the procedure for doing a patch release with this change?

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 28, 2020

@jonhoo I'll put one out this week! Are you under time pressure? If so, I'll can try and do one tomorrow (the 28th).

@jonhoo

This comment has been minimized.

Copy link
Contributor Author

jonhoo commented Jan 28, 2020

Nope, no rush on my end. I'm using it in https://github.com/mit-pdos/noria, but we're fine using patches in Cargo.toml to git for the time being :)

udoprog added a commit to udoprog/unicycle that referenced this pull request Jan 28, 2020
udoprog added a commit to udoprog/unicycle that referenced this pull request Jan 28, 2020
@jonhoo

This comment has been minimized.

Copy link
Contributor Author

jonhoo commented Feb 3, 2020

@cramertj Any update on a release for this? Would be nice to get rid of the patches.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Feb 3, 2020

Yup, I'm doing one today!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.