When using RedisStore, how to prevent app from hanging?

[abcd-ca]

Hi @maxcountryman, I've moved this question over from Issues as requested. So, my Axum app is hanging in a deadlock, no panic once there are enough concurrent requests, which doesn't need to be a lot. You have already mentioned that this is expected behaviour when instantiating a RedisStore without specifying a ReconnectPolicy. I have two somewhat related questions.

1. I think this is what you're referring to for ReconnectPolicy, though I am not confident about which settings to use. My REST API gets bursts of traffic. It's for an event with thousands of people actively poking around in the app for a while each day for a month, making many requests per user.

    let reconnect_policy = ReconnectPolicy::Constant {
        attempts: 1,      // initial attempts
        max_attempts: 10, // max attempts
        delay: 5_000,     // delay in milliseconds
        jitter: 100,      // jitter in milliseconds
    };

    let redis_client = RedisClient::new(
        redis_config,
        None,
        None,
        Some(reconnect_policy),
    );

2. Prior to adding tower-sessions, I already added a Redis client for sharing an immutable app state between my routes which is really currently only for convenience to hold access to my Postgres and Redis clients. That redis client is using mobc-redis which provides a connection pool. The connection pool for redis seems to be a different paradigm than what tower_sessions::fred::RedisClient uses; a ReconnectPolicy. I found that the mobc-redis client wasn't compatible with tower-sessions so now I have two different redis client crates. Which one is better and can you recommend a direction for refactoring to just one of them?

Thank you!

[maxcountryman]

Thanks for moving this over--I appreciate it!

Let me try to address your questions in order:

1. Unfortunately I'm also a new user to fred too, so I'm offering my best guess here (plus a little experience I have using this with Upstash): generally speaking, that seems like a good starting point. That said, you may want to see what advice fred has to offer in their docs or community forums. It's also a little unclear to me if the client should fail the way it does: I'm not sure if it's a deadlock or something else, but the behavior is again as you've described and seems to be directly related to fred.
2. We've picked fred as the client for our default implementation. However, your case is likely to not be at all uncommon and what we recommend here is that you either implement your own session store, which you can do with SessionStore or if you think fred is better fit for your needs explore using it instead of mobc-redis. If you're happy with mobc-redis (if it's already working well for you), I would probably suggest the former approach before going with the latter.

[maxcountryman]

Also this example from the fred repo might give you an idea of the possible patterns for building the client.

[abcd-ca]

Thanks @maxcountryman, that was a helpful lead but so far I still have the same problem even with the ReconnectPolicy:

        // redis ------------ start
        let redis_config = RedisConfig::from_url(&redis_url)?;
        let mut redis_builder = Builder::from_config(redis_config);
        // or modify values in place (creating defaults if needed)
        redis_builder
            // .with_performance_config(|config| {
            //     config.auto_pipeline = true;
            // })
            .with_config(|config| {
                config.version = RespVersion::RESP3;
                config.fail_fast = true;
            })
            .with_connection_config(|config| {
                config.tcp = TcpConfig {
                    nodelay: Some(true),
                    ..Default::default()
                };
                config.internal_command_timeout = Duration::from_secs(10);
            })
            .set_policy(ReconnectPolicy::new_exponential(0, 100, 30_000, 2))
            .set_performance_config(PerformanceConfig::default());

        // let redis_pool = redis_builder.build_pool(10)?;
        let redis_client = redis_builder.build()?;

I can reproduce the hang by only hitting an Axum REST endpoint that does not use session store and my other routes are commented out. If I remove the .layer(session_service) from my main Router, the drill load test succeeds.

Here is my drill config, which is executed with:

drill --benchmark benchmark.yml --stats --no-check-certificate

benchmark.yml

---
# Testing notes
# 500 concurrency with 10000 iterations crashes the health check with Redis session store. Doesn't crash with MemoryStore

concurrency: 500
base: 'http://localhost:8080/api'
iterations: 10000
rampup: 2

plan:
  - name: Health Check
    request:
      url: /health_check

The nice thing with the fred builder is that I can easily also get a RedisPool instead of a RedisClient so I will try that next to see if it's any more robust. That said, you had suggested that the lack of a ReconnectPolicy was your suspected culprit. Do you see anything else I've missed?

[abcd-ca]

I just tried using a store that I just whipped up that is a modified version of RedisStore but uses a RedisPool:

use async_trait::async_trait;
use time::OffsetDateTime;
use tower_sessions::fred::clients::RedisPool;
use tower_sessions::fred::{prelude::KeysInterface, types::Expiration};
use tower_sessions::{fred, session::Id, Session, SessionStore};

/// An error type for `RedisPoolStore`.
#[derive(thiserror::Error, Debug)]
pub enum RedisPoolStoreError {
    /// A variant to map to `fred::error::RedisError` errors.
    #[error("Redis error: {0}")]
    Redis(#[from] fred::error::RedisError),

    /// A variant to map `rmp_serde` encode errors.
    #[error("Rust MsgPack encode error: {0}")]
    RmpSerdeEncode(#[from] rmp_serde::encode::Error),

    /// A variant to map `rmp_serde` decode errors.
    #[error("Rust MsgPack decode error: {0}")]
    RmpSerdeDecode(#[from] rmp_serde::decode::Error),
}

/// A Redis session store.
#[derive(Debug, Clone)]
pub struct RedisPoolStore {
    client: RedisPool,
}

impl RedisPoolStore {
    pub fn new(client: RedisPool) -> Self {
        Self { client }
    }
}

#[async_trait]
impl SessionStore for RedisPoolStore {
    type Error = RedisPoolStoreError;

    async fn save(&self, session: &Session) -> Result<(), Self::Error> {
        let expire = Some(Expiration::EXAT(OffsetDateTime::unix_timestamp(
            session.expiry_date(),
        )));

        self.client
            .set(
                session.id().to_string(),
                rmp_serde::to_vec(&session)?.as_slice(),
                expire,
                None,
                false,
            )
            .await?;

        Ok(())
    }

    async fn load(&self, session_id: &Id) -> Result<Option<Session>, Self::Error> {
        let data = self
            .client
            .get::<Option<Vec<u8>>, _>(session_id.to_string())
            .await?;

        if let Some(data) = data {
            Ok(Some(rmp_serde::from_slice(&data)?))
        } else {
            Ok(None)
        }
    }

    async fn delete(&self, session_id: &Id) -> Result<(), Self::Error> {
        self.client.del(session_id.to_string()).await?;
        Ok(())
    }
}

This is the only change from my previous message for the redis builder:

let redis_pool = redis_builder.build_pool(60)?; // note, the default max in my redis server is 10000
// let redis_client = redis_builder.build()?;

It is also hanging in the load test, unfortunately. I can see that it must be correct to some degree since it compiles and I can use with the app as a single user via the browser interface without any issues. It still seems to just be the concurrency.

[abcd-ca]

I can also cause it to hang with the load tester set to,

concurrency: 10
iterations: 500

[maxcountryman]

I've not been able to reproduce with drill and the redis-store example. I do notice that drill shows ~200-300 failed requests, but it doesn't seem to hang.

I ran the load test about ten times and consistently see the same results. Here's a sample output: https://gist.github.com/maxcountryman/62efaff1c1c7dbfeeb9529f8289be410

Note that I tried this on bb06fd2 (main) and b16031e, with no significant difference (the latter may be ~1,000 req/sec faster, but it could also be noise). I'm using a MBP M2 Max to run the load tests.

[maxcountryman]

It seems like the latest release may have addressed an issue someone else was having with Redis (and other stores). Does updating to 0.6.0 fix this for you?

[abcd-ca]

@maxcountryman Wow, yes, that fixes my problem. I was even able to update the load testing benchmark settings to 1000 concurrent requests for 100,000 requests and it won't hang. Thank you for following up about this. I'm curious to know so that I can learn – what was the cause before?

The only thing outstanding is that this is the type of report I get from Drill:

Time taken for tests      4.6 seconds
Total requests            100000
Successful requests       99301
Failed requests           699
Requests per second       21564.06 [#/sec]
Median time per request   30ms
Average time per request  36ms
Sample standard deviation 22ms
99.0'th percentile        101ms
99.5'th percentile        111ms
99.9'th percentile        125ms

I don't know why there are any failed requests

[abcd-ca]

I just tried again after removing the session store layer from my Axum router and I still get the failed requests so maybe tower-sessions isn't responsible for that. It could be a flaw in drill or that I'm exceeding what's physically possible for concurrency on an M1 chip. If you have insight, I'd appreciate hearing it but it does sound like it's outside of the scope of crate. Thanks again!

P.S.

I asked the question over at the Drill repo, FYI

[abcd-ca]

FYI no difference testing redis pool vs redis client (without pool). Neither hang now and failed requests from Drill are about the same.

[maxcountryman]

I believe the cause was some shared memory access we were using dashmap for: this only worked for single-node deploys anyway so removing it seems okay for now. (We may be able to recover safe concurrent session access this was enabling if we move toward lazy sessions, which is what I'm looking at currently.)

[maxcountryman]

I believe this is resolved as of a few releases ago. If not, please let us know.