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

rt: split internal runtime::Handle concerns #5022

Merged
merged 2 commits into from
Sep 16, 2022
Merged

Conversation

carllerche
Copy link
Member

The runtime::Handle struct is part of the public API but is also used internally. This has created a bit of tension. An earlier patch made defined Handle as a private struct in some cases when rt is not enabled.

This patch splits out internal handle concerns into a new scheduler::Handle type, which will only be internal. This also defines a Handle type for each scheduler variant. Eventually, the per-scheduler Handle types will replace the per-scheduler Spawner types, but more work is needed before we can make that change.

The `runtime::Handle` struct is part of the public API but is also used
internally. This has created a bit of tension. An earlier patch made
defined Handle as a private struct in some cases when `rt` is not
enabled.

This patch splits out internal handle concerns into a new
`scheduler::Handle` type, which will only be internal. This also defines
a `Handle` type for each scheduler variant. Eventually, the
per-scheduler `Handle` types will replace the per-scheduler `Spawner`
types, but more work is needed before we can make that change.
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Sep 16, 2022
@carllerche carllerche added C-maintenance Category: PRs that clean code up or issues documenting cleanup. A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime and removed R-loom Run loom tests on this PR labels Sep 16, 2022
@carllerche
Copy link
Member Author

I would suggest disabling whitespace in the diff. runtime::Handle is mostly an indentation change.

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Sep 16, 2022
@carllerche carllerche removed the R-loom Run loom tests on this PR label Sep 16, 2022
@Noah-Kennedy
Copy link
Contributor

I think it might be advantageous to not have overlapping names for these handles. Could we name the internal handle "SchedulerHandle" or "InternalHandle" or something else?

@carllerche
Copy link
Member Author

@Noah-Kennedy I can look into cleanup later, but IMO it is more idiomatic to use mod prefix. ie. scheduler::Handle. This avoids the "stuttering" that you would get with scheduler::SchedulerHandle. See this discussion: rust-lang/api-guidelines#66

@Noah-Kennedy
Copy link
Contributor

@carllerche Good point, that is more readable.

@carllerche carllerche merged commit ebeb78e into master Sep 16, 2022
@carllerche carllerche deleted the restructure-rt-handle branch September 16, 2022 21:05
dbischof90 pushed a commit to dbischof90/tokio that referenced this pull request Oct 1, 2022
The `runtime::Handle` struct is part of the public API but is also used
internally. This has created a bit of tension. An earlier patch made
defined Handle as a private struct in some cases when `rt` is not
enabled.

This patch splits out internal handle concerns into a new
`scheduler::Handle` type, which will only be internal. This also defines
a `Handle` type for each scheduler variant. Eventually, the
per-scheduler `Handle` types will replace the per-scheduler `Spawner`
types, but more work is needed before we can make that change.
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 C-maintenance Category: PRs that clean code up or issues documenting cleanup. M-runtime Module: tokio/runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants