Reactor thread crash with "Bad file number"

[jothan]

I can trigger a panic in the reactor thread by refreshing a page quickly with the hyper HTTP server that is running over async-io-mini (ESP32, ESP-IDF std config).

My guess would be that a closed file descriptor is added to the select() call.

Panic message:

thread 'async-io-mini' panicked at async-io-mini-e233321a2e3791f3/b28e2c7/src/reactor.rs:336:28:
called `Result::unwrap()` on an `Err` value: Os { code: 9, kind: Uncategorized, message: "Bad file number" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

abort() was called at PC 0x401553c2 on core 0
0x401553c2 - panic_abort::__rust_start_panic::abort
    at /home/joe/.rustup/toolchains/esp/lib/rustlib/src/rust/library/panic_abort/src/lib.rs:49

[ivmarkov]

I agree!

File descriptor modifications should wait for the reactor to notify them that the modification is applied before proceeding with e.g. closing the file descriptor.

Let me see if I can create a unit test that reliably reproduces the bug...

[ivmarkov]

@jothan - there is in the meantime this branch which does pass the unit tests, so maybe you want to give it a try.

Unfortunately, this is a "blind fix" based on my hypothesis that the code crashes when the file descriptor is closed (on Drop) - yet - the FD is still used in the select syscall - but I have not yet confirmed this hypothesis, as I can't come up with a unit test (yet) which is demonstrating the problem you are exhibiting.

Granted, I am still trying on Linux, and there, the select call might be more resilient to invalid (closed) file handles (though that sounds weird I agree) or it is just a timing issue and I can't hit the correct timing when the problem occurs.

I'll keep trying.

[jothan]

I could not get it to crash after applying your branch, after pounding the ESP32 with the browser for a good 2 minutes, so it looks like you are on the right track.

Looking at the code, maybe only the deregister needs to wait for the reactor ? ....I have not taken the time to look at the code in much detail.

[ivmarkov]

I could not get it to crash after applying your branch, after pounding the ESP32 with the browser for a good 2 minutes, so it looks like you are on the right track.

Good news! :)
In the meantime, I'm crudely altering the reactor code to make it crash on Linux, and it seems this thing (select on Linux) is really resilient w.r.t. invalid file handles - I close the handle, it is still select-ed, yet, the select call does not return an error? Go figure... :)

Looking at the code, maybe only the deregister needs to wait for the reactor ? ....I have not taken the time to look at the code in much detail.

You are likely right, but I think - "better safe than sorry" and "better simple than complex" if I can rephrase it so. All modifications are currently protected with the same piece of code/logic as we are not chasing performance here.

[ivmarkov]

But if you would like to do a formal code review - be my guest - I've opened this branch as a PR now.

[ivmarkov]

I'm about to give up w.r.t. the unit test. :(
On Linux, the following code does not crash:

#[test]
fn wait_close() -> io::Result<()> {
    future::block_on(async {
        REACTOR.start().unwrap();
        REACTOR.register(11).unwrap();

        Timer::after(Duration::from_millis(1000)).await;

        Ok(())
    })
}

... where 11 is obviously an invalid file handle. Extending MAX_REGISTRATIONS from 20 to 200 and setting 131 instead of 11 givens the same "successful" outcome.

How about I just finish the PR and merge?

[jothan]

I'm about to give up w.r.t. the unit test. :( On Linux, the following code does not crash:

#[test]
fn wait_close() -> io::Result<()> {
    future::block_on(async {
        REACTOR.start().unwrap();
        REACTOR.register(11).unwrap();

        Timer::after(Duration::from_millis(1000)).await;

        Ok(())
    })
}

... where 11 is obviously an invalid file handle. Extending MAX_REGISTRATIONS from 20 to 200 and setting 131 instead of 11 givens the same "successful" outcome.

How about I just finish the PR and merge?

I've had a look at the code, I can't say I understand the details, but it looks fine from here. It definitely makes things better.

I'm really surprised that linux lets invalid FDs by like that. 🤷

[jothan]

Thank you so much for fixing this quickly, it is really appreciated.

[ivmarkov]

Alright - I've merged the branch into master now.

[jothan]

I'm getting an out of memory error on master, I suspect this commit:

65c1e17

[jothan]

confirmed:

log::info!("listener fd: {}", listener.as_fd().as_raw_fd());

Reports that the FD has a value of 48 using ESP-IDF.

[ivmarkov]

Sorry about that! Indeed a regression - fixed.

Some details:
The maximum file handle number in ESP IDF is 63 (because FD_SETSIZE there is 64), so we need to support there file handles with numbers from 0 up to 63 inclusive.

It is another topic:

• How many file handles ESP IDF allows to be opened simultaneously (trying to dig that up, but we actually don't care about that, and that number is surely <= FD_SETSIZE)
• More important is - how many file handles WE (as in async-io-mini) would allow the user to create (actually, to have alive simultaneously at any point in time) with the Async wrapper. This is MAX_REGISTRATIONS which used to be 20, and is now = FD_SETSIZE = 64. This is a trade-off between reactor size and more flexibility, which we can control in future with a feature. With MAX_REGS = 20 (before) the reactor size is ~ 800 bytes; with MAX_REGS = 64 (now), it is ~ 3 times bigger.

[ivmarkov]

Fix here: 6378246