-
Notifications
You must be signed in to change notification settings - Fork 195
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
Set up Tokio runtime in main() #3082
Conversation
Each entrypoint to the container bits sets up a tokio runtime, which is inefficient and duplicative. We're also likely to start using Rust async in more places. Instead, create a Tokio runtime early in our `main`, and change the CLI entrypoint to be an `async fn`. The other setup of a runtime we have is deep inside the sysroot upgrader bits, also for the container. In this case we actually have another thread (distinct from the main one where we set up Tokio) created by C/C++, so we need to pass a `tokio::runtime::Handle` across, and call `enter()` on it to set up the thread local bindings to access tokio async from there. I was initially looking at properly handling `GCancellable` with tokio and wanted to clean this up first.
@@ -16,7 +16,7 @@ pub(crate) fn import_container( | |||
// TODO: take a GCancellable and monitor it, and drop the import task (which is how async cancellation works in Rust). | |||
let repo = repo.gobj_wrap(); | |||
let imgref = imgref.as_str().try_into()?; | |||
let imported = build_runtime()? | |||
let imported = Handle::current() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have misunderstood the rest of the FFI changes around. I would expect this function and the one below to take a TokioEnterGuard
(or maybe directly an Handle
) to make sure they are always called beneath a tokio runtime.
Did I misunderstand the surrounding changes on the C++ side, or did you just forget to plumb a new input argument here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could plumb through the Handle
but I think this is a fine pattern; Handle::current()
will panic if not called on a tokio runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, I forsee us using more async on the Rust side, and it'd be annoying to have to thread another argument through all the API calls to do it. Tokio already has the runtime in a thread-local for precisely this kind of thing (AIUI). It's very unlikely we somehow fail to set up the runtime on the worker thread. (And eventually, I think we'll move the worker thread to be spawned by Rust, which would solve this problem too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, I thought you exposed the tokio_handle_get()
in order to move from a runtime panic to a compiler-enforced safety net.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's more from the other side, we need it because the C++ side needs to hold onto the Handle
reference in order to pass it to the new C++ thread and install it there.
Each entrypoint to the container bits sets up a tokio runtime,
which is inefficient and duplicative. We're also likely
to start using Rust async in more places.
Instead, create a Tokio runtime early in our
main
, andchange the CLI entrypoint to be an
async fn
.The other setup of a runtime we have is deep inside the
sysroot upgrader bits, also for the container. In this
case we actually have another thread (distinct from the main one
where we set up Tokio) created by C/C++, so we need to pass
a
tokio::runtime::Handle
across, and callenter()
on itto set up the thread local bindings to access tokio async
from there.
I was initially looking at properly handling
GCancellable
with tokio and wanted to clean this up first.