runtime (threadpool) does not shutdown when thread panics

[steffengy]

extern crate futures;
extern crate tokio;

fn main() {
    // Runtime does not shutdown ever... program never terminates
    tokio::run(futures::lazy(|| -> futures::future::FutureResult<(), ()> {panic!("test")}));
}

If this is expected behavior (requiring the user to handle panics), this should definitely documented.
Else and probably better, this should be handled.

[carllerche]

Thanks! This is definitely an omission.

[steffengy]

@carllerche
Do you have any solution in mind for that?

In this case one could expect shutdown_on_idle (which tokio::run calls) to propagate the panic.
In other cases, where work is done concurrently, you won't necessarily want a single thread panic to shutdown the entire threadpool/runtime.

futures-cpupool simply uses "CpuFuture" as a "backchannel" (which we do not have for tokio::run) for this,
since you'd expect to handle a failure when polling it anyways.

So some ideas that could work from my point of view so far:

• Shutdown entire runtime by default, require explicit usage of futures catch_unwind to prevent this in cases where you want it (e.g. webservers, ...)
• make tokio::run (and other potential methods that should behave this way) use catch_unwind and rethrow the panic after idle shutdown

[carllerche]

ah, sorry... i'm looking into it right now.

It seems to be exposing some other limitations.

[carllerche]

I opened #214.

This shuts down the worker threads, but many not result in all spawned futures to be dropped. These futures aren't leaked per se as they are tied to their notification handles, but this is probably not what is intended.

[steffengy]

@carllerche
#214 doesn't fix the example I posted above.
This is because the worker panics, which only drops the Worker but not the runtime.
Then the runtime blocks until the workers become idle, which they can't because they are dead.

[carllerche]

Ah right, i read it wrong...

[carllerche]

I pushed #216 which wraps the task with a catch_unwind. This will not bubble up to the caller of tokio::run as the future passed to run is just another future submitted to the thread pool.

If the caller wishes to know if the future panicked, that should be handled by the caller.

[steffengy]

@carllerche That sounds reasonable and works.

[Marwes]

@carllerche

If the caller wishes to know if the future panicked, that should be handled by the caller.

What would be the best way of handling this? The following is the best/first thing I could come up with :/

pub fn run_no_panic_catch<F>(fut: F)
where
    F: Future<Item = (), Error = ()> + Send + ::std::panic::UnwindSafe + 'static,
{
    use std::sync::{Arc, Mutex};

    let error_result = Arc::new(Mutex::new(None));
    {
        let error_result = error_result.clone();
        tokio::run(
            fut.catch_unwind()
                .map_err(move |err| {
                    *error_result.lock().unwrap() = Some(err);
                })
                .and_then(|result| result),
        );
    }

    if let Some(err) = Arc::try_unwrap(error_result).unwrap().into_inner().unwrap() {
        panic!(err)
    }
}

It also seems that #216 have suppressed some tests in tokio itself like

tokio/tests/runtime.rs

Line 49 in 68b82f5

tokio::run(create_client_server_future());

which will not fail due to panics anymore.

[carllerche]

@Marwes you can detect if a panic happened by adding a Drop impl for the future. However, if you want to prevent panics for a future, that is another issue. Why do you need to prevent a future to panic?

The threadpool already internally catches panics, so it should not kill a thread.

[Marwes]

The code is from a test so if it panics for any reason I want to treat that as a test failure.

[Marwes]

I guess panics from futures dispatched onto a separate thread might still be supressed in that case though? 🤔 Is there a reliable way to detect panics even if they are caught on a thread boundary? Or must tests be written in a way to fail despite that?

[carllerche]

What you probably want is to implement a future by hand with a drop impl that checks is the Future was dropped without yielding a value