Skip to content

ref(scheduler): Make InnerSchedulerState an enum #4251

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

Merged
merged 4 commits into from
Apr 3, 2023

Conversation

flub
Copy link
Contributor

@flub flub commented Mar 30, 2023

This is more verbose, but makes reasoning about things easier.

Followup from #4246.

#skip-changelog

This is more verbose, but makes reasoning about things easier.
@flub flub requested review from link2xt and iequidoo March 30, 2023 10:50
enum InnerSchedulerState {
Started(Scheduler),
#[default]
Stopped,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like Stopped is not needed, it's just Paused { started: false, pause_guard_count: 0 }

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say pause_guard_count = 0 is not a valid state. Because then it is not clear what started = true, pause_guard_count = 0 means.

Or we can offset pause_guard_count by 1, so 0 means that there is a single guard and it counts additional guards.

Copy link
Collaborator

@iequidoo iequidoo Mar 30, 2023

Choose a reason for hiding this comment

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

But we can say also that started = true, pause_guard_count = 0 is an invalid state, remove Stopped and probably get less code.

Maybe indeed it's better to get rid of invalid states by renaming pause_guard_count to, say, extra_pause_guard_cnt or just extra_pauses_cnt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could make pause_guard_count a NonZeroU32 so you can't create the Paused state with pause_guard_count=0. It avoids this "do we count from 0 or 1" ambiguity.

I'd leave Stopped as an explicit state, I think having stopped and paused as distinct cases makes it easier to reason about.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another option is:

enum InnerSchedulerState {
    Started(Scheduler),
    Stopped(usize), // the number of pause guards
    Paused(NonZeroUsize)
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is easier to count from 0 than use NonZeroU32. NonZeroU32 is very tricky, for example it has saturating_add, but no saturating_sub. It is not nice to work with at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option is:

enum InnerSchedulerState {
    Started(Scheduler),
    Stopped(usize), // the number of pause guards
    Paused(NonZeroUsize)
};

Not sure I follow this one.

src/scheduler.rs Outdated
ref started,
ref mut pause_guards_count,
} => {
*pause_guards_count = pause_guards_count.saturating_sub(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we start counting with 0 instead of 1, this can go to the else branch. If count is 0 and there are no additional guards, then stop, otherwise reduce the count.

This avoids having to have a meaning for pause_guards_count == 0.  It
does however introduce a bunch of unwraps.  All of them but one are
safe:  When incrementing the pause_guards_count by one this could
panic if a lot of guards are taken out.  This is very unlikely in real
code though and probably means there's a bug elsewhere already.
@flub
Copy link
Contributor Author

flub commented Mar 30, 2023

Updated with NonZeroUsize (usize makes much more sense for this, should have done that already before).

As the commit message points out: this adds a bunch of unwrap calls. All but one are safe: when incrementing the pause_guard_count by one it could panic if we take out too many guards. But that probably means there is already some other buggy code.

I'm not too fond of it though, if you panic in a tokio task you probably just quietly scream in the void. Our application setup does not handle this and does not log those panics so things would just break subtly while carrying on. The alternative is to allow .pause() to return a Result. Maybe that's not so bad.

@flub flub requested review from link2xt and iequidoo March 31, 2023 09:10
ref started,
ref mut pause_guards_count,
} => {
if *pause_guards_count == NonZeroUsize::new(1).unwrap() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These .unwrap()s are not needed if you count from zero.

The point of NonZeroUsize is to use 0 as a value for None when it is wrapped as Option<NonZeroUsize>, but otherwise it is just harder to work with, you need all these .get()s and .unwrap()s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know NonZero* types do optimise storage (though was really disappointed to see it doesn't do it in this case, IIUC it totally could put the bool and NonZeroUsize in a single usize-sized word - not that this matters for us though). I still think it's a nice newtype, though it could be nicer to use indeed.

I'm a bit undecided. I think the current version is reasonable, but it does have unwrap calls. The signature has to change to return a Result<IoPausedGuard> in any case to be safe. If you both vote for usize + counting from zero I'm happy to change it.

Unfortunately I can not get the compiler to warn if you don't extract
the guard from the result.
@flub flub merged commit c6c20d8 into master Apr 3, 2023
@flub flub deleted the flub/pause-guard-inner-enum branch April 3, 2023 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants