EnterGuard should impl Drop? (Compiler allows EnterGuard to exist after Runtime is dropped)

[Gankra]

Version

tokio 1.24.2

Platform

All platforms(?)

Description

(Apologies if this has been filed before, I couldn't find it.)

My coworker was bringing up some tokio stuff and innocently wrote

pub fn initialize_tokio_runtime() -> Runtime {
    tokio::runtime::Builder::new_multi_thread()
        .worker_threads(1)
        .max_blocking_threads(128)
        .enable_all()
        .build()
        .expect("Initializing tokio runtime failed")
}

fn main() -> Result<()> {
    let _guard = initalize_tokio_runtime().enter();
    ...
}

Which doesn't work because Rust drops the temporary runtime and ruins your day...

Producing a runtime error!?!?

It seems the borrowchecker is Simply Too Smart and makes the lifetime/borrow on EnterGuard completely useless! The borrowchecker sees that the EnterGuard<'_> is outliving its borrow, but also it's never used... so it's fine. Non-lexical lifetimes say this is all A-OK.

Normally for a Guard type this wouldn't be a problem since the whole point of such a type is to do something on Drop, and a Drop impl is a use, so it would complain that Runtime doesn't live for the whole lifetime of _guard. So if you want the borrow to actually work, that's a simple fix.

Here's two playgrounds demonstrating this difference:

• Without Drop (Bad Use Compiles, Runtime shuts down before the test ends): https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=65c659ad0bc29b1048b171c2fef45a4e

• With Drop (Bad Use Doesn't Compile): https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7c7af32eb0b5160246b3c778bad009cd

[Gankra]

Addendum: it was pointed out to me that there is an inner type which implements Drop, which means EnterGuard needs_drop but somehow the compiler is still being "too smart" about it. This is especially surprising!

Reduced example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=bdb2142766d51175261375fa7bff8640

start test 1
  runtime born!
    guard born!
  runtime dead!
end test 1
    guard dead!

[Gankra]

A quick bisect on godbolt reveals that this Drop/borrow behaviour was introduced in Rust 1.36.0 (non-lexical lifetimes being introduced circa 1.31.0 with Rust 2018).

Ah actually 1.36.0 was when NLL became available on Rust 2015. Passing --edition=2018 to 1.31.0 gets you NLL and lets this code compile. So yeah this is simply A Behaviour Of NLL.

[Gankra]

An alternative fix for adding an extra dummy Drop impl would be to give the inner type that Actually Impls Drop the lifetime. That makes the compiler agree the two are Definitely Connected.

[Darksonn]

Well, that's certainly annoying. Adding a Drop impl seems good to me.

[Gankra]

I quickly hacked up a patch to do this but it appears that the runtime's own builder """use after frees""":

https://github.com/tokio-rs/tokio/blob/c90757f07a1b15d7e26a710003d8e98a83db1ffc/tokio/src/runtime/builder.rs#L1056-L1062

The compiler is understandably upset with this code now.

(edit to be clear: genuinely no idea what's up with this code and what the right fix is)

[Darksonn]

Adding a scope around those lines like this should fix it:

{
    let _enter = handle.enter();
    launch.launch();
}

[Gankra]

That does indeed fix things, but now a lot of tests are mad. A ton of your tests manually shut down the runtime which takes it by value while the rt is still live:

https://github.com/tokio-rs/tokio/blob/c90757f07a1b15d7e26a710003d8e98a83db1ffc/tokio/tests/rt_handle_block_on.rs#L460-L465

I'm not sure if you're ok with breaking this kind of pattern, and since the tests are all specifically testing wonky usage I think someone more experienced needs to handle this if they want to "fix" it.

Here's the main fix to add to tokio/src/runtime/runtime.rs (at line 35)

impl Drop for EnterGuard<'_> {
    fn drop(&mut self) {
        // This Drop impl exists so that borrowck isn't too smart and accepts that
        // the lifetime in EnterGuard and the destructor of its inner SetCurrentGuard
        // are connected.
        //
        // Without this destructor the compiler thinks it's fine for EnterGuard to
        // outlive the Handle it conceptually points to!
    }
}

[Darksonn]

That's super annoying. I'm not sure what the best solution here is. One option is that we just leave it as-is. There isn't any safety issue with entering the runtime context after shutting it down — you can do that by using tokio::runtime::Handle::enter.

[Gankra]

I agree, it's just a nasty footgun with a confusing error.