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

[draft] io: consume budget in Registration::readiness #4784

Closed
wants to merge 1 commit into from

Conversation

Noah-Kennedy
Copy link
Contributor

Fixes #4782.

@Noah-Kennedy Noah-Kennedy changed the title io: consume budget in Registration::readiness [WIP] io: consume budget in Registration::readiness Jun 22, 2022
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net M-coop Module: tokio/coop labels Jun 22, 2022
@Darksonn
Copy link
Contributor

This seems like it might make some operations consume budget twice.

@Noah-Kennedy Noah-Kennedy changed the title [WIP] io: consume budget in Registration::readiness io: consume budget in Registration::readiness Jun 22, 2022
@Noah-Kennedy Noah-Kennedy changed the title io: consume budget in Registration::readiness [draft] io: consume budget in Registration::readiness Jun 22, 2022
@Noah-Kennedy
Copy link
Contributor Author

Yes, I don't think this is actually where I want to put this.

@Noah-Kennedy
Copy link
Contributor Author

I'll finish this up in the morning, I'm a bit too sleepy to figure out where best to put it right now.

@Noah-Kennedy
Copy link
Contributor Author

@Darksonn I'm not actually sure where something we have could consume budget twice for the same work.

Also, is there any general guidelines for where should consume budget (which operations is it worthwhile for) or not? I was looking through where all we do budgeting vs don't and it doesn't seem super consistent to me.

@Noah-Kennedy
Copy link
Contributor Author

The CI failures I think have to do with tests assuming that something will always immediately yield Poll::Ready(), which shouldn't be guaranteed because we reserve the right to put budgeting into things.

@Darksonn
Copy link
Contributor

Generally, we consume one budget whenever a poll function returns Poll::Ready. However, I agree that this is less clear for readiness-based APIs.

@Darksonn Darksonn marked this pull request as draft July 20, 2022 11:25
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking closer at this, I believe the following should be enough to finish this PR.

Pin::new(&mut fut).poll(cx).map(Ok)
let x = Pin::new(&mut fut).poll(cx).map(Ok);

coop.made_progress();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
coop.made_progress();
if x.is_ready() {
coop.made_progress();
}

@fasterthanlime
Copy link
Contributor

@Noah-Kennedy do you have time to finish this up? If not, would you like me to take it over?

@Noah-Kennedy
Copy link
Contributor Author

I should be able to finish this up on Saturday. If you can get to this before then, go for it!

@Darksonn
Copy link
Contributor

Since this is a stale draft, I will close it for now.

@Darksonn Darksonn closed this Apr 16, 2023
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-coop Module: tokio/coop M-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tokio in current_thread not releasing open file handles once the limit is reached.
3 participants