Tokio: Change `max_threads` to `max_blocking_threads`

[afdw]

Version

└── tokio v0.2.22

Platform

Linux notebook 5.8.5-arch1-1 #1 SMP PREEMPT Thu, 27 Aug 2020 18:53:02 +0000 x86_64 GNU/Linux

Description

On a runtime with the same number of core_threads as max_thread executing tokio::task::spawn_blocking will produce a future that never completes. For example:

fn main() {
    tokio::runtime::Builder::new()
        .threaded_scheduler()
        .core_threads(4)
        .max_threads(4)
        .build()
        .unwrap()
        .block_on(async {
            eprintln!("begin");
            tokio::task::spawn_blocking(|| {
                eprintln!("spawn_blocking");
            })
            .await
            .unwrap();
            eprintln!("end");
        });
}

Playground
Output:

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 1.22s
     Running `target/debug/playground`
begin
/playground/tools/entrypoint.sh: line 11:     7 Killed                  timeout --signal=KILL ${timeout} "$@"

I am not sure that this behavior is desired. Maybe requiring core_threads < max_threads instead of core_threads ≤ max_threads or panicking or immediately returning an error in tokio::task::spawn_blocking would be better?

[Darksonn]

We should be able to look at this as part of #2720.

[arthurprs]

I discovered this the hard way yesterday :sob:

[Darksonn]

@carllerche Should we disallow this configuration as part of v1?

[carllerche]

We could make max_threads -> max_blocking_threads or something like that, then max_threads automatically becomes core_threads + max_blocking_threads.

[dekellum]

Those semantic changes seem fine (though I'm not sure necessary). I request that it remains allowed that max_blocking_threads be configured to zero (0). Of course, it would be better if that case caused an error return (or even just a panic) on call to spawn_blocking, _rather than never completing_, as this issue suggests.

[Darksonn]

I think that with the semantic change that @carllerche suggests, it is fine to allow it to be configured to zero.

[dekellum]

How hard would an assert or panic be to add on the call to spawn_blocking for that case? (Assuming you agree.)

[dekellum]

In a use case of mine, I don't want to implicitly be using spawn_blocking. (I'd prefer a panic to let me know if I missed such use.)

[Darksonn]

That doesn't sound too bad. One point to consider is whether it should panic on spawn, or just return a JoinHandle that fails immediately with e.g. a cancelled error like spawning already does on a runtime that has shut down.

[dekellum]

Either is fine by me. I don't have a strong preference. I'd hope in the latter case, something might log or be passed back up through any APIs using spawn_blocking underneath.

[dekellum]

3287 just panics on max_blocking_threads(0) which is unfortunate IMO, per my above use case and request.