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.
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.
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.
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.
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.
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::Handleacross, and callenter()on itto set up the thread local bindings to access tokio async
from there.
I was initially looking at properly handling
GCancellablewith tokio and wanted to clean this up first.