tokio::run doesn't propogate panics from futures

[canndrew]

I assumed the following test would fail, but it currently passes.

#[test]
fn my_test() {
    tokio::run(future::lazy(|| -> Result<_, _> {
        panic!("oh no!")
    }))
}

Is this expected behaviour? It doesn't seem like it should be.

[aep]

got the same issue. setting panic hook to exit in every test is my workaround: https://doc.rust-lang.org/std/panic/fn.set_hook.html

[carllerche]

@canndrew this is the intent. tokio::run starts the system. It would be as if a panic on a thread caused all threads to crash.

You might be wanting block_on, which (should) propagate panics.

Closing for now. Let me know if this is incorrect and I can open again.

[canndrew]

Sorry, you're right, I misunderstood what tokio::run was for. However it might be nice if there was a tokio::block_on convenience function, rather than having to start a runtime manually (this would be especially useful for tests).

[leoyvens]

It would be as if a panic on a thread caused all threads to crash.

@carllerche That's exactly what I want for tests. A configuration for the Runtime to propagate any panic would be nice.

[carllerche]

@leodasvacas does block_on not satisfy your case?

[leoyvens]

@carllerche No because it does not compose. If I block_on(future) then future might spawn a new task where a panic would not fail the test.

[jeff-hiner]

I hate to drag back open an old issue, but this is still a problem. I can't find a way to reliably capture threadpool panics in tests. I have a codebase liberally using tokio::spawn that I'm in the middle of fuzzing, and certain inputs cause those spawned futures to error or panic. From the standpoint of tokio::spawn there appears to be no behavioral difference between a passed Future returning an err or an ok. I guess this is like being unable to react to panics in a detached thread? What's the joinable thread alternative, and is it documented somewhere I'm not seeing?

[carllerche]

I'd be fine having a config option passed to runtime::Builder that allows configuring the behavior.

[canndrew]

How about making it so tokio propagates panics in debug mode but not release mode?

[carllerche]

We can probably start w/ a builder option, then decide after that what the defaults should be?

[jeff-hiner]

Yeah, differing behavior in debug and release modes I think is asking for trouble.

[ry]

Panics should crash the program. This feature continues to introduce bugs in Deno. (Most recently denoland/deno#2098)

[carllerche]

I will re-open since the action item is to add a config option.

I believe the story will be better in 0.2.

[leoyvens]

I'd also like to be able to override the global panic behaviour per-task, so that selected tasks (and child tasks) still catch panics.

[ry]

Shouldn’t it be the responsibility of user code to catch their panics? I can imagine a web server wanting to serve special html 500 error code pages. At tokio’s layer of abstraction, catching panics isn’t helpful because it doesn’t know how to properly service them.

[jeff-hiner]

I think what I was getting at was more that tokio::run is passed something which may error (or panic, because any code in Rust can legally panic) but it does not have a return type at all. That's a design decision. This inherently delegates error handling to the runtime which, as you've said, is not equipped to deal with these problems.

I would expect the ergonomics to be set up so that the runtime returns a Result<(), ...> in a way that makes errors from tokio::spawn tasks available. I expect tokio to offer behavior consistent with std::thread::JoinHandle::join. I can spawn a thread and join it, in which case I'll get a Result wrapping any panics. Or I can spawn a thread and drop its JoinHandle, in which case panics will be silently dropped. There are cases for using both within a single program.

For your webserver example, the main server should propagate any errors upstream terminating the runner, and the client tasks should probably just discard any errors. If you drop every thread panic on the floor, you can wind up with scenarios where your main server thread panics, other spawned tasks run forever, and your program just keeps chugging along unable to service requests. If you insist that every thread panic terminates the runner, then panics in client threads will terminate your server. This is safer, but less ergonomic.

Right now there's no way to tell the runner which threads should be treated as detached and which ones should terminate the runtime. I can see an implementation with tokio::spawn and tokio::spawn_detached calls, where panics in the former terminate the runner and panics in the latter are silently discarded.

[carllerche]

I don't think this issue is relevant anymore:

• Runtimes will allow configuring how panics are handled (by default, they will swallow, which is OK because...)
• spawn! will return a handle annotated with must_use. panics are propagaged via this (Provide spawn macro #1180).
• tokio::run is removed in favor of an attribute macro.

[carllerche]

If I am wrong, please post here and I can update the appropriate issue.

[mimoo]

@jeff-at-dwelo don't you have issues fuzzing concurrent code anyway? As it might throw the fuzzer off?