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

Builder::worker_threads doesn't panic when used with a current_thread runtime #4773

Closed
hds opened this issue Jun 17, 2022 · 0 comments · Fixed by #4849
Closed

Builder::worker_threads doesn't panic when used with a current_thread runtime #4773

hds opened this issue Jun 17, 2022 · 0 comments · Fixed by #4849
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-runtime Module: tokio/runtime T-docs Topic: documentation

Comments

@hds
Copy link
Contributor

hds commented Jun 17, 2022

Version

tokio v1.19.2 (also master branch)

Platform

Linux sync-z2 4.15.0-162-generic #170-Ubuntu SMP Mon Oct 18 11:38:05 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Description

The documentation for the function Builder::worker_threads states that if it is used
with current_thread runtime it will panic. However it doesn't panic, it appears to
ignore the value set by the call instead.

Documentation:
https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html#method.worker_threads

This test demonstrates the behaviour (it would live in tokio/tests):

#![warn(rust_2018_idioms)]
#![cfg(feature = "full")]

use tokio::runtime::Builder;

#[test]
#[should_panic]
fn test_worker_threads_panics() {
    let rt = Builder::new_current_thread()
        .worker_threads(4)
        .build()
        .unwrap();

    rt.block_on(async {
        tokio::spawn(async {}).await.unwrap();
    });
}

The test fails as nothing panics.

Additionally, the documentation for worker_threads contains 2 panic sections.
The other panic section describes a case that does panic (val = 0).

The 2 options I see here are:

  1. Leave the functionality as it is (silently ignore worker_threads for a current_thread runtime) and fix the documentation
  2. Change the functionality so that it does panic (ideally directly within worker_threads itself) and also fix the documentation so that all panics are described in a single heading

I would lean towards option 1 as this makes it easier to choose the runtime Kind in the builder at execution time. It will also not break any existing code.

@hds hds added A-tokio Area: The main tokio crate C-bug Category: This is a bug. labels Jun 17, 2022
@Darksonn Darksonn added T-docs Topic: documentation M-runtime Module: tokio/runtime labels Jun 17, 2022
hds added a commit to hds/tokio that referenced this issue Jul 20, 2022
The documentation for the runtime `Builder::worker_threads` function
incorrectly stated that it would panic if used when constructing a
`current_thread` runtime. In truth, the call to the function has no
effect.

Since adding the described panic to the code could cause new panics in
existing code using tokio, the documentation has been modified to
describe the existing behavior.

Refs: tokio-rs#4773
Darksonn pushed a commit that referenced this issue Jul 23, 2022
The documentation for the runtime `Builder::worker_threads` function
incorrectly stated that it would panic if used when constructing a
`current_thread` runtime. In truth, the call to the function has no
effect.

Since adding the described panic to the code could cause new panics in
existing code using tokio, the documentation has been modified to
describe the existing behavior.

Refs: #4773
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-bug Category: This is a bug. M-runtime Module: tokio/runtime T-docs Topic: documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants