Failing to spawn an arbiter thread should not panic creator thread

[Sytten]

We have had reports of some users running out of file handle, causing tokio to panic the new thread which in turns triggered this unwrap (https://github.com/actix/actix-net/blob/master/actix-rt/src/arbiter.rs#L153) and killed the caller thread (main in my case).
I think it would be better to offer a safe API (we can keep backward compatibility with an unwrap).

[dontloseyourheadsu]

Can I try solving this one? Do you have contribution guidelines, I heard github projects can have them?

[robjtede]

@dontloseyourheadsu feel free! up to you, just let us know how you get on :)

[dontloseyourheadsu]

@robjtede Thanks!

Just to be sure, the issue is about making a new function, lets say for example "try_with_tokio_rt" and instead of panicking I wrap it in an error (A Result<T, E>) and return that instead?

I suppose I should add some tests for the good and bad paths, but is there anything else I should add? For example, I noticed the ::new() uses the "with_tokio_rt", so maybe a ::try_new() would be good?

Note: Sorry for so many questions, this is literally my first github contribution that isn't my own project ^^"
Also, I added a small test that I will remove later, just to test the scenario that is described. Is this the scenario mentioned?

#[test]
fn prove_arbiter_crash_on_runtime_failure() {
    // similar setup as the test that also uses with_tokio_rt
    let _sys = System::new();

    // i tried to exhaust fds using sockets
    let mut handles = Vec::new();
    
    let path = if cfg!(windows) { "NUL" } else { "/dev/null" };

    loop {
        match std::fs::File::open(path) {
            Ok(f) => handles.push(f),
            Err(e) => {
                println!("Exhausted file handles after {} opens. Last error: {}", handles.len(), e);
                break;
            }
        }
    }

    // I try to create a new Arbiter, which should panic the main thread due to inability to create necessary runtime resources.
    let result = std::panic::catch_unwind(|| {
        Arbiter::new()
    });

    // and well, this is just my cleanup
    drop(handles);

    assert!(result.is_err(), "Arbiter::new() should have panicked the main thread due to FD exhaustion");
}

which seems the case because it panicked in the line 153 like the issue says, but im not sure... here is the test output:
running 1 test
Exhausted file handles after 1048561 opens. Last error: Too many open files (os error 24)

thread 'actix-rt|system:0|arbiter:0' panicked at actix-rt/src/arbiter.rs:102:53:
Cannot create new Arbiter's Runtime.: Os { code: 24, kind: Uncategorized, message: "Too many open files" }
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

thread 'prove_arbiter_crash_on_runtime_failure' panicked at actix-rt/src/arbiter.rs:153:25:
called Result::unwrap() on an Err value: RecvError
test prove_arbiter_crash_on_runtime_failure ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 18 filtered out; finished in 1.20s

[JohnTitor]

Yeah, I'd take that approach to keep API compatibility.

And about test, it'd be better using sync::mpsc::channel and simulating tokio panic somehow.