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

make timeout robust against budget-depleting tasks #4314

Merged
merged 2 commits into from Dec 15, 2021

Conversation

BraulioVM
Copy link
Contributor

Refs: #4291 , #4300

Motivation

Up until now, if the future that we were applying a timeout to
consistently depleted the coop budget, the timeout never got a chance to
be evaluated. In the next call to poll, the underlying future would be
polled and it would once again deplete the budget. In those circumstances,
timeouts would not be respected. This can be surprising to people, and
in fact it was in #4291

Solution

The solution is to make a budget exception with timeout if it was the
underlying future that depleted the budget.

I guess there are still pathological cases where the budget is almost depleted
before polling the delay, and it gets depleted while polling the delay, resulting in
the timeout never being hit as well. How do we want to go about that? I could simply do:

if had_budget_before {
  coop::with_unconstrained(poll_delay)  
}

instead of taking into consideration whether the underlying future depleted (or not) the budget.

Testing

I have added a test along the lines of the original #4291 issue.

Up until now, if the future that we were applying a timeout to
consistently depleted the coop budget, the timeout never got a chance to
be evaluated. In the next call to `poll`, the underlying future would be
polled and it would once again deplete the budget. In those circumstances,
timeouts would not be respected. This can be surprising to people, and
in fact it was in tokio-rs#4291 .

The solution is to make a budget exception with `timeout` if it was the
underlying future that depleted the budget.

Refs: tokio-rs#4291 , tokio-rs#4300
@BraulioVM
Copy link
Contributor Author

The solution was inspired by @Darksonn 's comment in #4300 (comment)

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-time Module: tokio/time labels Dec 11, 2021
@Darksonn Darksonn added the R-loom Run loom tests on this PR label Dec 11, 2021
@BraulioVM
Copy link
Contributor Author

Anything else I can do here?

@Darksonn
Copy link
Contributor

No, thanks.

@Darksonn Darksonn merged commit 54e6693 into tokio-rs:master Dec 15, 2021
carllerche added a commit that referenced this pull request Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-time Module: tokio/time R-loom Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants