Feature request: return the current number of connections in pool from retain

[evanrittenhouse]

I'd like to track how many connections are currently in a given Pool so that I can gather some data and adjust the pool size. I'm pruning connections based on an idle timeout, so retain is perfect here. However, it takes an Fn, which prevents mutable state borrows. This means I can't do something like:

            let mut removed = 0;
            clone.retain(|_, metrics| {
                let retain = metrics.last_used() < idle_timeout;

                if !retain {
                    removed += 1;
                }

                retain
            });

The one way I can think of doing this is having removed be an AtomicU64, since that gives us access to fetch_add which takes an immutable receiver. That opens us up to unnecessary overhead though, what with all the atomic operations in an already-blocking closure.

It seems like the calculation is already done, we just have to return guard.size from retain():

    pub fn retain(&self, f: impl Fn(&M::Type, Metrics) -> bool) {
        let mut guard = self.inner.slots.lock().unwrap();
        let len_before = guard.vec.len();
        guard.vec.retain_mut(|obj| {
            if f(&obj.obj, obj.metrics) {
                true
            } else {
                self.manager().detach(&mut obj.obj);
                false
            }
        });
        guard.size -= len_before - guard.vec.len();
    }

I'm happy to raise a PR, or if there are any workarounds, please let me know!

[bikeshedder]

I wonder if we might want a RetainResult structure like so:

struct RetainResult<T> {
    pub retained: usize,
    pub removed: Vec<T>,
}

Maybe someone wants to remove a couple of objects from the pool and still do something meaningful with them before finally dropping them for good?

btw. keep in mind that the number of retained objects is NOT the current pool size. Objects currently borrowed from the pool are completely invisible to this method.

[evanrittenhouse]

keep in mind that the number of retained objects is NOT the current pool size

Great point! We're just looking to see if our pools are too large/small by checking the idle timeout. Currently borrowed objects shouldn't hit that.

RetainResult is a great idea! I can raise a PR today

[evanrittenhouse]

Well, or would it be easiest to change the Fn bound to FnMut? @bikeshedder I'll defer to you - just wondering because retained may be a bit misleading if there are invisible objects, and we may need larger changes to the backing data store since the backing M::Type isn't Copy/Clone

[bikeshedder]

You are right. Changing this to a FnMut is a sensible change anyways.

I do like the idea with the RetainResult, too. It just makes sense to also return the removed objects.

Now that I think about it it could even improve the performance a bit as the drop() call of those objects could be deferred to a point after the Mutex lock is released. It's probably going to have minimal impact. I do like the idea of creating a more feature rich API and improving performance at the same time.

[evanrittenhouse]

I started looking into the FnMut stuff today. It looks like apply requires mutable access to f to call the actual function here, since FnMut uses call_mut under the hood:

    pub(crate) async fn apply(
        &self,
        inner: &mut ObjectInner<M>,
    ) -> Result<(), HookError<M::Error>> {
        for hook in &self.vec {
            match hook {
                Hook::Fn(f) => f(&mut inner.obj, &inner.metrics)?,
                Hook::AsyncFn(f) => f(&mut inner.obj, &inner.metrics).await?,
            };
        }
        Ok(())
    }

That means that we need mutable access to self, or we need to store the hook vector elsewhere. As far as storing it on self, we could either use a Mutex or a RwLock, but both of those could slow checkouts down since we'd need a write lock for mutating state.

One workaround I was thinking was that we could use a RwLock and have additional SyncFnMut/AsyncFnMut variants on Hook. That'd let us could either grab read locks indiscriminately, if the user specifies the "read" variety. If the user wants to use the "write" variety, they'd need to specify the *Mut variant, where we can say that this may slow things down under high load since we need mutable access to the hook vector.

Though honestly, the more I think about it, the worse it is to potentially block on every create/recycle due to a hook. Curious what your opinion is, @bikeshedder (I've been meaning to look at RetainResult separately, I'll give that a look hopefully soon as well. Sorry for the delay!)

[bikeshedder]

I just gave this a try myself and changing the method signature does seam to do the the trick:

• Change retain() function to accept a FnMut #374

-pub fn retain(&self, f: impl Fn(&M::Type, Metrics) -> bool) {
+pub fn retain(&self, mut f: impl FnMut(&M::Type, Metrics) -> bool) {

Were you trying something different?

[bikeshedder]

I have merged both the FnMut PR and added the RetainResult return value. The next deadpool release will include this feature.

As this is not a breaking change - unless someone explicitly expected the retain(...) return value to be unit type () - I will probably release this as deadpool 0.12.2.

[evanrittenhouse]

Thanks!