recover when OS fails to spawn a new thread

[gwik]

Motivation

As described in #2309 the runtime panics when trying to spawn a new thread and the OS have reaches the limit of threads / processes.

This can be observed on a Linux machine decreasing the limit with ulimit -u and running the program in #2309.

As noted by @Darksonn there is a pool of threads, and failing to spawn a new one is not mandatory for the program to make progress.

Solution

Ignore the temporary errors when failing to spawn a new thread from the OS.
The task will be naturally scheduled by threads already in the pool.

Fixes: #2309

[gwik]

I looked at how I could test this, but it let me puzzled...
Guidance appreciated.

[hawkw]

looks good to me!

[Darksonn]

There are some CI failures that you will have to investigate.

[gwik]

error[E0599]: no variant or associated item named `OutOfMemory` found for enum `ErrorKind` in the current scope
   --> /home/gwik/src/playground/tokio/tokio/src/runtime/blocking/pool.rs:311:62
    |
311 |         std::io::ErrorKind::WouldBlock | std::io::ErrorKind::OutOfMemory
    |                                                              ^^^^^^^^^^^ variant or associated item not found in `ErrorKind`

Looks like OutOfMemory was added in 1.54 and therefore it fails on 1.49.
Anyway, I tried to simulate memory exhaustion with cgroup and the process gets OOM killed and I can't make pthread_create to fail with because of no memory available. Even when configuring with huge thread stack. So I guess it's ok to remove the OutOfMemory which wasn't part the issue in the first place anyway.

I confirmed that the fix is working on MacOS and this is a non-issue on Windows since there doesn't seem to be a limit of the number of threads (available memory is the limit). I tried on a VM and I couldn't trigger the bug.

About the test, I wonder if something like this would work on the CI:

• spawn a task A on the runtime that waits
• separately spawn (std::thread::spawn) as many threads as possible until the OS refuse to create one.
• spawn a task B on the runtime
• kill a thread spawn separately
• make task A finish
• verify that task B ran

I wonder how accurate and stable a test like this would be. I doubt it is worth. Ideas ?

[hawkw]

Looks like OutOfMemory was added in 1.54 and therefore it fails on 1.49.
Anyway, I tried to simulate memory exhaustion with cgroup and the process gets OOM killed and I can't make pthread_create to fail with because of no memory available. Even when configuring with huge thread stack. So I guess it's ok to remove the OutOfMemory which wasn't part the issue in the first place anyway.

It could be worth using autocfg or something to include the OutOfMemory variant in this code only on Rust >= 1.54.0, but this may not be particularly important if we can't easily reproduce a scenario where pthread_create fails with that error. If anyone does hit it in the wild, we can always come back and add Rust version detection in order to handle the OutOfMemory variant...

About the test, I wonder if something like this would work on the CI:

• spawn a task A on the runtime that waits
• separately spawn (std::thread::spawn) as many threads as possible until the OS refuse to create one.
• spawn a task B on the runtime
• kill a thread spawn separately
• make task A finish
• verify that task B ran
  I wonder how accurate and stable a test like this would be. I doubt it is worth. Ideas ?

I'm a bit skeptical about running a test like this on CI, it seems potentially flaky. This code seems simple enough that I'm not sure if it's really worth adding such a complex test or not. If we are going to do this, though, I think it's better to lower the thread limit using ulimit, rather than just spawning a huge pile of threads until we run out...

[gwik]

It could be worth using autocfg or something to include the OutOfMemory variant in this code only on Rust >= 1.54.0, but this may not be particularly important if we can't easily reproduce a scenario where pthread_create fails with that error. If anyone does hit it in the wild, we can always come back and add Rust version detection in order to handle the OutOfMemory variant...

I agree it's fine.

I'm a bit skeptical about running a test like this on CI, it seems potentially flaky. This code seems simple enough that I'm not sure if it's really worth adding such a complex test or not. If we are going to do this, though, I think it's better to lower the thread limit using ulimit, rather than just spawning a huge pile of threads until we run out...

I'm very skeptical too, using ulimit on my machine I had to play with many different settings to find the value that reproduce the crash. It's probably going to be more of a burden than an asset to have this test.

If it's fine for you like this, it's fine for me. Thanks for the review.

[gwik]

@hawkw is this ok to merge ?

[hawkw]

@hawkw is this ok to merge ?

I was hoping to get a second approval from another maintainer, just to make sure I haven't missed anything, but I think it seems good.

[Darksonn]

No concerns from me.