Pool grows beyond max_size, hits open files limit

[liskin]

Here's a straightforward reproducer:

use std::{panic, thread, time::Duration};

fn main() {
    delay_main_panic();

    for _ in 1..250 {
        let srv = mockito::Server::new();
        std::mem::drop(srv);
    }

    println!("All good!");

    // maybe some fds will get closed? probably not.
    thread::sleep(Duration::from_secs(5));
}

// workaround for panic messages from different threads getting mixed together
fn delay_main_panic() {
    let def_panic_hook = panic::take_hook();
    panic::set_hook(Box::new(move |info| {
        if thread::current().name() == Some("main") {
            thread::sleep(Duration::from_secs(1));
        }
        def_panic_hook(info)
    }));
}

This results in the following on a Linux system with ulimit -n being 1024:

thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Error { kind: ServerFailure, context: Some("Too many open files (os error 24)") }', /home/tomi/src/mockito/src/server.rs:238:56
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { kind: ServerFailure, context: Some("the server is not running (context: receiving on a closed channel)") }', src/main.rs:7:19

---

liskin@ea7e583 demonstrates why this is happening:

• self.created is never being incremented, therefore self.created < self.max_size always evaluates to true
• thus a new Server is always created and added to the pool whenever a Server is requested, regardless of the pool size and regardless of whether any recycled servers are available
• every server uses 5 file descriptors (and at least one thread), so ~200 calls to mockito::Server::new exhaust the limit (on MacOS, the default limit is much lower, so just 50 calls exhausts it)

Ideally, the pool would first try to satisfy a request using any available (recycled) servers, thus keeping the resource (file descriptor, thread, memory) usage minimal. If there are no available servers, only then would it create new ones on demand.

(I believe this is how deadpool behaves. What was wrong with it?)

(Unrelated, but when playing around with the code I did this and it compiles and tests pass. Being quite new to Rust, I'm guessing I'm missing an important reason for those clones… What is it?)

[lipanski]

self.created is never being incremented

🙄 that was silly of me, I disabled it a while back when I was investigating some hanging tests and it seems I forgot to put it back.

Ideally, the pool would first try to satisfy a request using any available (recycled) servers, thus keeping the resource (file descriptor, thread, memory) usage minimal. If there are no available servers, only then would it create new ones on demand.

well my intention was similar, but I would fill up the pool first and then start reusing. guess the pool size could be lowered to 50 (once I fix the created counter), to fit the file descriptor limit on MacOS.

(I believe this is how deadpool behaves. What was wrong with it?)

my main intention was to keep dependencies to a minimum, but I was also trying out various different pooling frameworks at the time and with deadpool I noticed some hanging async tests (though it wasn't due to deadpool after all, as I figured out later).

(Unrelated, but when playing around with the code I did [this (https://github.com/liskin/mockito/commit/3f211387e04000238bfa304ba314dc7b1179e25d) and it compiles and tests pass. Being quite new to Rust, I'm guessing I'm missing an important reason for those clones… What is it?)

see the documentation on Clone and Deref. in your code, it seems Deref is enough (in most cases) since the Mock struct owns the state and the reads should be fine on the Deref value while the write methods expose new objects anyway (so no need to keep track of the references). will have a closer look but I think your fixes are good 👍

[liskin]

well my intention was similar, but I would fill up the pool first and then start reusing. guess the pool size could be lowered to 50 (once I fix the created counter), to fit the file descriptor limit on MacOS.

Out of curiosity, what's the reasoning behind filling up the pool first rather than reusing aggressively?

my main intention was to keep dependencies to a minimum

Fair enough. It is actually nice to see people doing this intentionally. :-)

[lipanski]

Out of curiosity, what's the reasoning behind filling up the pool first rather than reusing aggressively?

it was just more simple to implement but I'm open to the idea.

PRs are welcome of course 😸

[lipanski]

Fixed and released in 1.1.0. I've set the server pool size to 50. If you need to bypass the pool, you can use the Server::new_with_port methods.

[xjewer]

pool size of 50 is still a bit of aggressive for macos, since default fd limit is 256.
considering tests can read some other files, it will easily go over the limit.

Maybe allow passing a configurable pool?

Out of curiosity, what's the reasoning behind filling up the pool first rather than reusing aggressively?

it was just more simple to implement but I'm open to the idea.

Or even shall we rewrite the pool to reuses servers more aggressively, rather than feeling it up first.

[kornelski]

I'll make a PR for it.

[lipanski]

@xjewer @kornelski fixes released in 1.2.0