DynamicRedisPool blocked on wait_for_connect

[JamesPatrickGill]

Description

I am attempting to use a dynamic pool in an actix web app (btw I am using a v4 beta so am on the correct tokio version) and my code hangs on wat_for_connect consistently. I know similar things have happened previously but as those were closed issues I started this one.

Reproduction

To reproduce I am able to just run the code from the example here https://github.com/aembke/fred.rs/blob/main/examples/dynamic_pool.rs

Here is my app main

use fred::pool::DynamicRedisPool;
use fred::prelude::*;

#[tokio::main]
async fn main() -> Result<(), RedisError> {
    let config = RedisConfig {
        // whether to skip reconnect logic when first connecting
        fail_fast: true,
        // server configuration
        server: ServerConfig::new_centralized("127.0.0.1", 6379),
        // whether to automatically pipeline commands
        pipeline: true,
        // how to handle commands sent while a connection is blocked
        blocking: Blocking::Block,
        // an optional username, if using ACL rules
        username: None,
        // an optional authentication key or password
        password: None,
        // optional TLS settings
        tls: None,
        // whether to enable tracing
    };
    // the max size isn't a hard limit - it just determines the size of the client array when the pool is initialized
    let pool = DynamicRedisPool::new(config, None, 5, 10);

    println!("I am printed");
    let _ = pool.connect();
    println!("Me too");
    let _ = pool.wait_for_connect().await?;
    println!("Not Me :(");

    // modify the size of the pool at runtime
    let (new_client, _) = pool.scale_up().await;
    if let Some(old_client) = pool.scale_down(true).await {
        assert_eq!(new_client.id(), old_client.id());
    }

    for client in pool.clients() {
        println!("Client ID {} in pool.", client.id());
    }

    // due to the locking required by the resizing operations the Deref trait cannot be used with this pool implementation.
    // if modifications to the pool are not required at runtime the static pool is usually easier to use
    let _ = pool.next().get("foo").await?;
    let _ = pool.next().set("foo", "bar", None, None, false).await?;
    let _ = pool.next().get("foo").await?;

    // if the pool can be empty a function exists that will lazily create a new client, if needed.
    // if the pool is not empty this just calls `next` without creating a new client.
    let _ = pool.next_connect(true).await.get("foo").await?;

    let _ = pool.quit_pool().await;
    Ok(())
}

Hope this helps and please ask any questions.

System

MacOS, Redis via Docker, Rust 1.57

[aembke]

Hi @JamesPatrickGill,

Which fred version are you using? There was a bug related to this in versions < 4.2.2.

In the meantime I'll try your repro and see if I can come up with anything.

Also, out of curiosity, what is your use case for the dynamic pool? I spoke to a number of folks recently about this and heard pretty consistent feedback that the dynamic pool is not really useful due to not implementing Deref<Target = RedisClient>, so I was planning on removing it for 5.0.0 (but keeping the static pool). But if it's useful to you I'll bring it back.

[JamesPatrickGill]

Hi @aembke. Thanks for getting back to me so quickly.

The version of fred is 4.3.1

Honestly I am in just experimenting and was just testing it out. For more context I am currently working on an actix4 web server using a mobc pool to manage my redis connections but the provided redis implementation was a little out of date. When I went looking around and found fred and saw it had some pooling capabilities built in so I thought it was worth a dive.

My use case basically requires a pool that will maintain a number of connections that can be used by different http handlers so that each one doesn't have to create a new client. A key requirement being that this server will be long running so reconnecting dropped connections should be smooth. From my understanding all of this is covered by the static pool so will likely go with that anyway. Was just investigating the other pool as the runtime scaling sounded interesting :).

[aembke]

Ah ok yeah that makes sense. I've seen a few cases where folks see the dynamic pool, they like the flexibility to scale it up or down, but when they go to actually use it they run into a bunch of ergonomics issues due to the lack of the Deref trait implementation.

While I don't want to leave any bugs in versions 4.x I'm about a week away from 5.0.0 coming out, so I might just table this if you're not actively using it. I still need to figure out if it's worth leaving the DynamicPool interface in the code at all, but if I do I'll make sure this kind of thing gets resolved.

[aembke]

@JamesPatrickGill it turns out this was just a broken example. When you call connect on the dynamic pool you need to await the response. I've updated the example in the soon-to-be-released 4.3.2,

[JamesPatrickGill]

Thank you for getting back to me, I should have noticed that!