Detached tasks in tests

[Darksonn]

What should we do about detached tasks in tests? Can we help people out by detecting silent mistakes?

[TheButlah]

This caught me off guard today - my code is using task::spawn without joining on the result because I don't want to block until the task finishes. But it meant that a bunch of panicking tasks were getting through CI without anyone noticing

[kstrafe]

Can we remove the catch_unwind from the harness if we set up a runtime with something like builder.catch_unwind(false)?

[carllerche]

In theory, I am OK w/ the idea of adding a catch_unwind configuration to the runtime builder. However, implementing this would not be trivial for the multi-threaded scheduler. The scheduler must maintain correctness, so the panic must be caught, the runtime cleanly shutdown, and the panic bubbled up to the caller. Of course, what is the "caller"? Is it block_on? If so, which block_on is it? There can be multiple concurrent calls to Runtime::block_on.

I would read a proposal.

[kstrafe]

@carllerche would it be acceptable to panic when building a multi-threaded scheduler that has been configured to not catch_unwind?

[Darksonn]

It's also not obvious for the current thread scheduler. That scheduler can also have multiple concurrent calls to block_on.

[kstrafe]

Since it's only useful for testing atm, why not let that be unspecified, and let the particular thread that happens to run into the panic just panic?

[MikailBag]

Alternatively, a new TestRuntime(tiny wrapper for single thread Runtime) can be introduced that
a) only allows single block_on
b) propagates panicks in tasks
And tokio::tes macro can use this TestRuntime in the expansion

[nashley]

Has anything changed on this front? Do tests still need to join/try_join each task's handle in order to catch panics?
It'd be nice if this was a warning at least.

[Darksonn]

For the case of panics, #4770 was recently merged, but it's not yet released.

[Darksonn]

Closing in favor of #4516.

[Ekleog]

It seems to me like #4516 is (implemented on tokio_unstable, and) focusing mostly on a generic mechanism to make unhandled panics have some behavior.

I'm thinking tokio::test should just default to having all panics trigger a test failure. As a consequence, it'd become possible to more easily test multi-task setups, without requiring the tokio_unstable flag as tokio::test would just default to this behavior.

OTOH, it does mean a change in behavior for tokio::test, which in itself could be considered semver-breaking, though I'd argue it's actually a bugfix.

That said, if you think it doesn't make sense I won't insist. Just wanted to raise the point that #4516 is neither necessary nor sufficient to stabilize for this to work better for testing use cases :)

[Darksonn]

OTOH, it does mean a change in behavior for tokio::test, which in itself could be considered semver-breaking, though I'd argue it's actually a bugfix.

It's way too breaking of a change to make that argument.