Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: Panic propagation for test code #3217

Closed
kstrafe opened this issue Dec 5, 2020 · 2 comments
Closed

testing: Panic propagation for test code #3217

kstrafe opened this issue Dec 5, 2020 · 2 comments
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-runtime Module: tokio/runtime

Comments

@kstrafe
Copy link

kstrafe commented Dec 5, 2020

Related issue: #2699.

Summary
The proposal aims to improve the testability of applications and libraries which spawn tasks internally by creating a new scheduler that doesn't capture panics.

Motivation
Tokio tasks mimic std::thread behavior by capturing panics and only propagating these when the associated JoinHandle is joined. The usage of OS-level threads and tokio tasks however differs in the sense that applications and libraries may liberally spawn the latter, whilst usually making a thread pool of the former.

To make good tests of such libraries and applications we typically avoid threading at all to remove nondeterminism, however, with tokio this might not be possible. We need to allow tests to fail on panics in detached tasks to thoroughly test these libraries and apps.

Proposal
The proposed changes are:

impl Builder {
    fn new_test() -> Builder { ... }
}

In addition to copying the basic_scheduler code but changing the way spawn constructs a harness, in this case, harnesses are constructed with an alternative vtable where poll does not capture panics.

Changes for the end-user of Tokio
This change is an additive feature put behind the rt-test flag. It does not impact existing tokio users in any way, nor does it impact performance by not adding any new branches or extra code to run for those who do not use the test scheduler (with the exception of a few matches in the runtime, but that should be negligible). The test scheduler yields a Runtime object such that the API is unchanged.

Risks
Risk of breaking API if this scheduler is removed.

Open questions
Running the test scheduler in different threads may nondeterministically cause a panic to propagate in the thread that runs a future that happens to panic. This makes panics appear in an unspecified thread. Is this desirable?

Can this cause UB due to the panic happening inside the vtable call? Are there invariants that must be upheld?

It's not yet known what other features we want to add to the test scheduler, or whether we want #[tokio::test] to use the test scheduler.

@kstrafe kstrafe added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Dec 5, 2020
@MikailBag
Copy link
Contributor

As was mentioned on original thread, such an API can be footgun in presence of multiple threads or muktiple block_ons.

I propose making a new type TestRuntime, which does not expose APIs that are dangerous for ones' feet (i.e. always single thread and no block_on allowed).

Alternatively, Runtime in test mode can just panic in those risky situations.

@Darksonn Darksonn added the M-runtime Module: tokio/runtime label Dec 6, 2020
@Darksonn
Copy link
Contributor

Closing in favor of #4516.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-runtime Module: tokio/runtime
Projects
None yet
Development

No branches or pull requests

3 participants