diff --git a/crates/deadpool-redis/src/config.rs b/crates/deadpool-redis/src/config.rs index f6fdf7be..05977aea 100644 --- a/crates/deadpool-redis/src/config.rs +++ b/crates/deadpool-redis/src/config.rs @@ -1,10 +1,13 @@ -use std::{fmt, path::PathBuf}; +use std::{fmt, path::PathBuf, time::Duration}; use redis::RedisError; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use crate::{CreatePoolError, Pool, PoolBuilder, PoolConfig, RedisResult, Runtime}; +use crate::{ + CreatePoolError, DEFAULT_CONNECTION_TIMEOUT, DEFAULT_RESPONSE_TIMEOUT, ManagerConfig, Pool, + PoolBuilder, PoolConfig, RedisResult, Runtime, +}; /// Configuration object. /// @@ -17,6 +20,10 @@ use crate::{CreatePoolError, Pool, PoolBuilder, PoolConfig, RedisResult, Runtime /// REDIS__POOL__MAX_SIZE=16 /// REDIS__POOL__TIMEOUTS__WAIT__SECS=2 /// REDIS__POOL__TIMEOUTS__WAIT__NANOS=0 +/// REDIS__CONNECTION_TIMEOUT__SECS=1 +/// REDIS__CONNECTION_TIMEOUT__NANOS=0 +/// REDIS__RESPONSE_TIMEOUT__SECS=0 +/// REDIS__RESPONSE_TIMEOUT__NANOS=500000000 /// ``` /// ```rust /// #[derive(serde::Deserialize)] @@ -47,6 +54,18 @@ pub struct Config { /// Pool configuration. pub pool: Option, + + /// Connection timeout applied when creating new Redis connections. + /// + /// Set to `None` to disable the timeout. Defaults to 1 second. + #[cfg_attr(feature = "serde", serde(default = "default_connection_timeout"))] + pub connection_timeout: Option, + + /// Response timeout applied when waiting for Redis responses. + /// + /// Set to `None` to disable the timeout. Defaults to 500 milliseconds. + #[cfg_attr(feature = "serde", serde(default = "default_response_timeout"))] + pub response_timeout: Option, } impl Config { @@ -70,15 +89,30 @@ impl Config { /// See [`ConfigError`] for details. pub fn builder(&self) -> Result { let manager = match (&self.url, &self.connection) { - (Some(url), None) => crate::Manager::new(url.as_str())?, - (None, Some(connection)) => crate::Manager::new(connection.clone())?, - (None, None) => crate::Manager::new(ConnectionInfo::default())?, + (Some(url), None) => self.build_manager(url.as_str())?, + (None, Some(connection)) => self.build_manager(connection.clone())?, + (None, None) => self.build_manager(ConnectionInfo::default())?, (Some(_), Some(_)) => return Err(ConfigError::UrlAndConnectionSpecified), }; + let pool_config = self.get_pool_config(); + Ok(Pool::builder(manager).config(pool_config)) } + fn build_manager( + &self, + params: T, + ) -> Result { + Ok(crate::Manager::new_with_config( + params, + ManagerConfig { + connection_timeout: self.connection_timeout, + response_timeout: self.response_timeout, + }, + )?) + } + /// Returns [`deadpool::managed::PoolConfig`] which can be used to construct /// a [`deadpool::managed::Pool`] instance. #[must_use] @@ -93,7 +127,7 @@ impl Config { Config { url: Some(url.into()), connection: None, - pool: None, + ..Default::default() } } @@ -104,9 +138,29 @@ impl Config { Config { url: None, connection: Some(connection_info.into()), - pool: None, + ..Default::default() } } + + /// Sets the connection timeout. + /// + /// Pass `Some(duration)` to set a specific timeout, or `None` to + /// disable it. + #[must_use] + pub fn with_connection_timeout(mut self, timeout: Option) -> Self { + self.connection_timeout = timeout; + self + } + + /// Sets the response timeout. + /// + /// Pass `Some(duration)` to set a specific timeout, or `None` to + /// disable it. + #[must_use] + pub fn with_response_timeout(mut self, timeout: Option) -> Self { + self.response_timeout = timeout; + self + } } impl Default for Config { @@ -115,10 +169,22 @@ impl Default for Config { url: None, connection: Some(ConnectionInfo::default()), pool: None, + connection_timeout: DEFAULT_CONNECTION_TIMEOUT, + response_timeout: DEFAULT_RESPONSE_TIMEOUT, } } } +#[cfg(feature = "serde")] +fn default_connection_timeout() -> Option { + DEFAULT_CONNECTION_TIMEOUT +} + +#[cfg(feature = "serde")] +fn default_response_timeout() -> Option { + DEFAULT_RESPONSE_TIMEOUT +} + /// This is a 1:1 copy of the [`redis::ConnectionAddr`] enumeration (excluding `tls_params` since it is entirely opaque to consumers). /// /// This is duplicated here in order to add support for the diff --git a/crates/deadpool-redis/src/lib.rs b/crates/deadpool-redis/src/lib.rs index ab035442..3e836c20 100644 --- a/crates/deadpool-redis/src/lib.rs +++ b/crates/deadpool-redis/src/lib.rs @@ -31,8 +31,12 @@ pub mod sentinel; use std::{ ops::{Deref, DerefMut}, sync::atomic::{AtomicUsize, Ordering}, + time::Duration, }; +const DEFAULT_CONNECTION_TIMEOUT: Option = Some(Duration::from_secs(1)); +const DEFAULT_RESPONSE_TIMEOUT: Option = Some(Duration::from_millis(500)); + use deadpool::managed; use redis::{ AsyncConnectionConfig, Client, IntoConnectionInfo, RedisError, RedisResult, @@ -142,6 +146,45 @@ impl std::fmt::Debug for Manager { } } +/// Connection configuration for the [`Manager`]. +/// +/// # Example +/// +/// ```rust +/// use std::time::Duration; +/// use deadpool_redis::{ManagerConfig, Manager}; +/// +/// let manager = Manager::new_with_config( +/// "redis://127.0.0.1", +/// ManagerConfig { +/// connection_timeout: Some(Duration::from_secs(5)), +/// response_timeout: None, +/// }, +/// ) +/// .unwrap(); +/// ``` +#[derive(Clone, Copy, Debug)] +pub struct ManagerConfig { + /// Timeout for establishing a connection. + /// + /// Set to `None` to disable. Defaults to 1 second. + pub connection_timeout: Option, + + /// Timeout for waiting for a response. + /// + /// Set to `None` to disable. Defaults to 500 milliseconds. + pub response_timeout: Option, +} + +impl Default for ManagerConfig { + fn default() -> Self { + Self { + connection_timeout: DEFAULT_CONNECTION_TIMEOUT, + response_timeout: DEFAULT_RESPONSE_TIMEOUT, + } + } +} + impl Manager { /// Creates a new [`Manager`] from the given `params`. /// @@ -157,18 +200,19 @@ impl Manager { } /// Creates a new [`Manager`] from the given `params` and - /// [`AsyncConnectionConfig`]. - /// - /// This allows configuring connection-level settings such as response timeouts and connection - /// timeouts. + /// [`ManagerConfig`]. /// /// # Errors /// /// If establishing a new [`Client`] fails. pub fn new_with_config( params: T, - connection_config: AsyncConnectionConfig, + config: ManagerConfig, ) -> RedisResult { + let connection_config = AsyncConnectionConfig::new() + .set_connection_timeout(config.connection_timeout) + .set_response_timeout(config.response_timeout); + Ok(Self { client: Client::open(params)?, connection_config: Some(connection_config), diff --git a/crates/deadpool-redis/tests/redis_connection_config.rs b/crates/deadpool-redis/tests/redis_connection_config.rs index 2bc5dad3..86e43c57 100644 --- a/crates/deadpool-redis/tests/redis_connection_config.rs +++ b/crates/deadpool-redis/tests/redis_connection_config.rs @@ -2,8 +2,8 @@ use std::time::Duration; -use deadpool_redis::{Manager, Pool, Runtime}; -use redis::{AsyncCommands, AsyncConnectionConfig}; +use deadpool_redis::Runtime; +use redis::AsyncCommands; use serde::{Deserialize, Serialize}; #[derive(Debug, Default, Deserialize, Serialize)] @@ -27,63 +27,44 @@ fn redis_url() -> String { Config::from_env().redis.url.unwrap_or_default() } -fn create_pool_with_config(connection_config: AsyncConnectionConfig) -> Pool { - let manager = Manager::new_with_config(redis_url(), connection_config).unwrap(); - Pool::builder(manager) - .max_size(1) - .runtime(Runtime::Tokio1) - .build() - .unwrap() -} - -fn create_pool_default() -> Pool { - let manager = Manager::new(redis_url()).unwrap(); - Pool::builder(manager) - .max_size(1) - .runtime(Runtime::Tokio1) - .build() - .unwrap() -} - -/// Verifies that `new_with_config` with `set_response_timeout(None)` allows commands that take -/// longer than the default 500ms timeout. -/// -/// Uses `BLPOP` on an empty list with a 1-second timeout. With the default `AsyncConnectionConfig` -/// (500ms response timeout), this would fail. With `set_response_timeout(None)`, it waits the full -/// second and returns nil. +/// Verifies that disabling the response timeout allows commands that take longer than the +/// default timeout. Uses `BLPOP` on an empty list with a 1-second server-side timeout. #[tokio::test] -async fn test_response_timeout_can_be_disabled() { - let config = AsyncConnectionConfig::new().set_response_timeout(None); - let pool = create_pool_with_config(config); +async fn test_response_timeout_disabled() { + let pool = deadpool_redis::Config::from_url(redis_url()) + .with_response_timeout(None) + .create_pool(Some(Runtime::Tokio1)) + .unwrap(); + let mut conn = pool.get().await.unwrap(); let result: Option<(String, String)> = conn - .blpop("deadpool/nonexistent_timeout_test_key", 1.0) + .blpop("deadpool/test_timeout_disabled", 1.0) .await .unwrap(); + assert_eq!(result, None); } -/// Verifies that the default `Manager::new` (without config) uses the redis crate's default -/// timeouts, which causes blocking commands exceeding 500ms to fail. +/// Verifies that setting an explicit response timeout causes commands exceeding it to fail. #[tokio::test] -async fn test_default_manager_times_out_on_slow_commands() { - let pool = create_pool_default(); +async fn test_response_timeout_causes_timeout() { + let pool = deadpool_redis::Config::from_url(redis_url()) + .with_response_timeout(Some(Duration::from_millis(100))) + .create_pool(Some(Runtime::Tokio1)) + .unwrap(); + let mut conn = pool.get().await.unwrap(); let start = std::time::Instant::now(); - let result: Result, _> = conn - .blpop("deadpool/nonexistent_default_timeout_key", 1.0) - .await; + let result: Result, _> = + conn.blpop("deadpool/test_timeout_short", 1.0).await; let elapsed = start.elapsed(); + assert!(result.is_err(), "expected timeout error"); assert!( - result.is_err(), - "expected timeout error with default config" - ); - assert!( - elapsed < Duration::from_millis(900), - "should have timed out before the 1s BLPOP completed, took {:?}", + elapsed < Duration::from_millis(500), + "should have timed out well before the 1s BLPOP, took {:?}", elapsed ); }