Repository: tokio-rs/tokio
File: tokio/src/runtime/blocking/pool.rs (Rust)

Code before:

        if shared.num_idle == 0 {
            // No threads are able to process the task.

            if shared.num_th == self.inner.thread_cap {
                // At max number of threads
            } else {
                assert!(shared.shutdown_tx.is_some());
                let shutdown_tx = shared.shutdown_tx.clone();

                if let Some(shutdown_tx) = shutdown_tx {
                    let id = shared.worker_thread_index;

                    if let Some(handle) = self.spawn_thread(shutdown_tx, rt, id) {
                        shared.num_th += 1;
                        shared.worker_thread_index += 1;
                        shared.worker_threads.insert(id, handle);
                    } else if shared.num_th == 0 {
                        // The OS refused to spawn the thread and there is no thread
                        // to pick up the task that has just been pushed to the queue.
                        panic!("Could not spawn any thread");
                    } else {
                        // OS temporarily failed to spawn a new thread.
                        // The task will be picked up eventually by a currently
                        // busy thread.
                    }
                }
            }
        } else {
            // Notify an idle worker thread. The notification counter
            // is used to count the needed amount of notifications
            // exactly. Thread libraries may generate spurious
            // wakeups, this counter is used to keep us in a
            // consistent state.
            shared.num_idle -= 1;
            shared.num_notify += 1;
            self.inner.condvar.notify_one();
        }

        Ok(())
    }

    fn spawn_thread(
        &self,
        shutdown_tx: shutdown::Sender,
        rt: &Handle,
        id: usize,
    ) -> Option<thread::JoinHandle<()>> {
        let mut builder = thread::Builder::new().name((self.inner.thread_name)());

        if let Some(stack_size) = self.inner.stack_size {

Review comment (bug):
it feels like the logic of whether to panic or not is now divided between here and the `spawn_thread` function. imo, it might be a little easier to follow if we either moved this logic into `spawn_thread` (which we might not be able to do, since this method locks `shared`), or if `spawn_thread` just returned a `Result<thread::JoinHandle, io::Error>`, and this code also included the logic for deciding whether an OS error is worth panicking over or not. (which I think is maybe the better option).

that might look something like this:

```suggestion
                    match self.spawn_thread(shutdown_tx, rt, id) {
                        Ok(handle) => {
                            shared.num_th += 1;
                            shared.worker_thread_index += 1;
                            shared.worker_threads.insert(id, handle);
                        },
                        Err(err) if is_temporary_os_thread_error(error) && shared.num_th > 0 => {
                            // OS temporarily failed to spawn a new thread.
                            // The task will be picked up eventually by a currently
                            // busy thread.
                        },
                        Err(e) => {
                            // The OS refused to spawn the thread and there is no thread
                            // to pick up the task that has just been pushed to the queue.
                            panic!("OS can't spawn worker thread: {}", e)
                        },
                    }
```
this way, `spawn_thread` _just_ handles actually trying to spawn a new thread, and isn't responsible for part of the decision of whether or not to panic.

what do you think?

Code after:

        if shared.num_idle == 0 {
            // No threads are able to process the task.

            if shared.num_th == self.inner.thread_cap {
                // At max number of threads
            } else {
                assert!(shared.shutdown_tx.is_some());
                let shutdown_tx = shared.shutdown_tx.clone();

                if let Some(shutdown_tx) = shutdown_tx {
                    let id = shared.worker_thread_index;

                    match self.spawn_thread(shutdown_tx, rt, id) {
                        Ok(handle) => {
                            shared.num_th += 1;
                            shared.worker_thread_index += 1;
                            shared.worker_threads.insert(id, handle);
                        }
                        Err(ref e) if is_temporary_os_thread_error(e) && shared.num_th > 0 => {
                            // OS temporarily failed to spawn a new thread.
                            // The task will be picked up eventually by a currently
                            // busy thread.
                        }
                        Err(e) => {
                            // The OS refused to spawn the thread and there is no thread
                            // to pick up the task that has just been pushed to the queue.
                            panic!("OS can't spawn worker thread: {}", e)
                        }
                    }
                }
            }
        } else {
            // Notify an idle worker thread. The notification counter
            // is used to count the needed amount of notifications
            // exactly. Thread libraries may generate spurious
            // wakeups, this counter is used to keep us in a
            // consistent state.
            shared.num_idle -= 1;
            shared.num_notify += 1;
            self.inner.condvar.notify_one();
        }

        Ok(())
    }

    fn spawn_thread(
        &self,
        shutdown_tx: shutdown::Sender,
        rt: &Handle,
        id: usize,