fred.rs
fred.rs copied to clipboard
aembke
[Bug] `WithOptions` is lost when you use `.replicas()`
Fred version - 9.0.3 Redis version - 6.2.4 Platform - max Deployment type - cluster|centralized
Describe the bug
In short, when you call .replicas() on a WithOptions<RedisPool>, the options are lost. This can lead to unexpected behavior, e.g. hanging if you had configured options like fail_fast or max_attempts. You can see more details here: https://github.com/aembke/fred.rs/discussions/255.
To Reproduce
pub fn get_redis_pool_options() -> Options {
Options {
max_attempts: Some(2),
timeout: Some(Duration::from_secs(4)),
fail_fast: true,
..Default::default()
}
}
let pool = Builder::from_config(config).build_pool(8).unwrap();
let options = get_redis_pool_options();
let pool = pool.with_options(&options);
let value: Option<String> = self
.redis_pool
// We don't care if the data from the cache reader is a bit stale, it is
// a good candidate for using replicas instead. If there are no replicas,
// the client will just use a normal node.
.replicas()
// We need to reapply the options because `.replicas()` does not keep the
// WithOptions wrapper: https://github.com/aembke/fred.rs/issues/256.
.with_options(&get_redis_pool_options())
.get(redis_key)
.await
.inspect_err(|e| error!(error = ?e, key=redacted_key, "Failed to get key from Redis"))
.with_context(|| format!("Failed to get key {}", redacted_key))?;
The options do not apply to the final request unless you include with_options / remove replicas.
Logs N/A
Hi @banool,
It looks like in this example you're binding the options to the pool via the pool variable, but then calling replicas() on self.redis_pool rather than pool. If you change self.redis_pool.replicas() to pool.replicas() does it still repro the issue?
Nevermind, I'm able to repro this with the changes above as well. Unfortunately this will require a breaking change to support, so I'm going to table it for now until 10.x. I'll add a warning in the docs about combining Replicas and WithOptions for now.
For what it's worth, you can set timeouts, max attempts, and max_redirections on the entire pool via the other config structs as well. That might help reduce some duplicate code in the meantime.
Yeah I realized that myself later, thanks for that, and the future fix for this issue!