RediStack
RediStack copied to clipboard
Redis retry configuration is potentially confusing
The Redis retry configuration is potentially confusing, and downright problematic for Vapor.
RedisConnectionPool.Configuration.init() is implemented like this. Note that it specifies a default timeout in the signature of 60 seconds, but if nil is passed in, it uses an internal timeout of 10 ms.
public init(
initialServerConnectionAddresses: [SocketAddress],
maximumConnectionCount: RedisConnectionPoolSize,
connectionFactoryConfiguration: ConnectionFactoryConfiguration,
minimumConnectionCount: Int = 1,
connectionBackoffFactor: Float32 = 2,
initialConnectionBackoffDelay: TimeAmount = .milliseconds(100),
connectionRetryTimeout: TimeAmount? = .seconds(60),
onUnexpectedConnectionClose: ((RedisConnection) -> Void)? = nil,
poolDefaultLogger: Logger? = nil
) {
self.initialConnectionAddresses = initialServerConnectionAddresses
self.maximumConnectionCount = maximumConnectionCount
self.factoryConfiguration = connectionFactoryConfiguration
self.minimumConnectionCount = minimumConnectionCount
self.connectionRetryConfiguration = (
(initialConnectionBackoffDelay, connectionBackoffFactor),
connectionRetryTimeout ?? .milliseconds(10) // always default to a baseline 10ms
)
self.onUnexpectedConnectionClose = onUnexpectedConnectionClose
self.poolDefaultLogger = poolDefaultLogger ?? .redisBaseConnectionPoolLogger
}
The Vapor wrapper around this passes nil down through its call stack.
It's not clear what the intent is in RediStack. Is a default timeout 60 seconds or 10 milliseconds? Certainly the public documentation implies it's 60 seconds, and only by a careful reading of the actual code can you determine that it ends up as 10 ms if you pass nil.
Big +1 to accept this update, just spent 30 minutes debugging why is my redis connection timing out.
This requires a breaking change. @0xTim could this be fixed in Vapor?
Yep happy to accept a PR to fix the default in Vapor