Runner changes: will use existing tokio::runtime or create one for itself and race condition fix

[conorbros]

Shotover's Runner will use an existing runtime if it is running inside one (e.g. in async tests) or create one for itself (same behaviour as before).

The Runtime builder was moved to another method (Runner::runtime) and we check if we can get a handle on the current runtime, otherwise create one and return the Handle and Runtime objects. run_spawn, run_block and with_observability_interface use this method to get a handle to start their tasks from. ShotoverManager was modified to hold a handle and runtime object. We have to store the runtime inside the ShotoverManager so that it does not go out of scope during tests and shut down.

I was also able to update tokio from 1.6.1 to 1.11.0.

[rukai]

Now that I can see the size of the race condition fix I can see that should have gone in a separate PR.
But the changes arent too complicated so I should be able to manage reviewing as is.

[rukai]

this looks like a race condition, but also this whole shutdown abstraction looks kind of weird.

[conorbros]

This code is adapted from the tokio mini-redis project.

[rukai]

I was wondering why there was so many comments... hehe.
Ill take a look

[rukai]

This snippet demonstrates why we need the check:

#[tokio::main]
async fn main() {
    let (tx, mut rx) = tokio::sync::watch::channel(false);

    tx.send(true).unwrap();

    rx.changed().await.unwrap();
    println!("borrow rx: {}", *rx.borrow());

    let mut rx2 = rx.clone();

    rx2.changed().await.unwrap(); //hangs here!
    println!("borrow rx2: {}", *rx2.borrow());
}

Alright, fair enough.

[rukai]

This snippet demonstrates why we need the check:

#[tokio::main]
async fn main() {
    let (tx, mut rx) = tokio::sync::watch::channel(false);

    tx.send(true).unwrap();

    rx.changed().await.unwrap();
    println!("borrow rx: {}", *rx.borrow());

    let mut rx2 = rx.clone();

    rx2.changed().await.unwrap(); //hangs here!
    println!("borrow rx2: {}", *rx2.borrow());
}

Alright, fair enough.

[rukai]

Is this doing what we think it is doing?
I dont see anything that actually uses the join handle in anyway.
Consider this screenshot with the ; removed.
We can see that its actually returning the JoinHandle (so the join handle must not be being used in any way)

If the block_in_place happens to give us a nice error maybe we can just stick it somewhere to give us that error.
Or maybe we can do whatever detection it is doing and give an even better error! Dont forget to include #[tokio::test(flavor = "multi_thread")] on your test! or something

[XA21X]

Yeah, I'm pretty sure this isn't doing anything. 😂

[conorbros]

Strange. With it removed shotover doesn't shutdown cleanly so I was under the impression it was blocking it until the join handle task finishes

[conorbros]

Yeah I think we need this instead. block_in_place lets us enter an async context and then we can use the handle to run the join handle future to completion.

tokio::task::block_in_place(move || {
    self.runtime_handle
    .block_on(self.join_handle.take().unwrap())
    .unwrap()
    .unwrap();
});

[rukai]

Ah that works.

However I'm thinking we should use block_in_place in Runner to assert we got a multithread runtime.
This way any user of Runner gets this assertion.
Using block_in_place is discouraged so we should revert back to entering the runtime rather than using block_in_place twice.

[XA21X]

Would it make sense to use Shutdown instead of watch everywhere else? The "whole shutdown abstraction looks kind of weird" is encapsulated by the abstraction so it seems a bit of a waste to reimplement it? Or is it important to keep the separation between what can initiate shutdowns and what can listen for it?

[rukai]

Yes, I think using Shutdown instead of watch is a good idea, but lets do that in a follow up PR.
Otherwise we risk hitting more issues and further blowing up the size of this PR.

[rukai]

LGTM