Kitura / Kitura-redis

Swift Redis library
Apache License 2.0
95 stars 26 forks source link

Redis connected property always returns true even after a disconnection #56

Closed emiliopavia closed 7 years ago

emiliopavia commented 7 years ago

Hi, I'm using the following code to make sure redis is connected before each request

router.all { _, _, next in
    if !redis.connected {
        redis.connect(host: "localhost", port: 6379) { (redisError: NSError?) in
            if let error = redisError 
                Log.error("\(error.localizedDescription)"
            }
        }
    }
    next()
}

This handler works well for the first request, after launching the app, but if it gets disconnected (for example if the redis server goes down for a while) the connected property always returns true, so I'm not able to recover it.

Thanks!

quanvo87 commented 7 years ago

@emiliopavia I would not check the state of the connection before each request:

Don't check first and then send. It's wasted effort and won't work anyway -- the status can change between when you check and when you send. Just do what you want to do and handle the error if it fails. https://stackoverflow.com/a/12411808/6706704

Nonetheless, the redis.connected property is misleading as it only reflects the result of trying to initially connect. I'll put in a PR to make it a computed property that returns the connection state of the underlying socket. However, this will still be inaccurate as I don't believe the underlying socket library Kitura-redis uses updates its connection state in the event of a disconnect.

The way to check if you can write to a socket is, surprisingly, to try and write to it :-) https://stackoverflow.com/a/1795213/6706704

emiliopavia commented 7 years ago

@quanvo87 thanks for your reply.

The reason for that is to try recovering when it fails, as I'm using a single Redis instance for the entire app (should I create a new instance for each request instead?).

quanvo87 commented 7 years ago

@emiliopavia You can try reconnecting on failures instead of checking the connection status before each request. I think just a single Redis instance can be used.

quanvo87 commented 7 years ago

Closing for now. Let us know if anything else comes up!