How can I ensure the redis.Client in the pool.Pool connected? #21

Closed
Akagi201 opened this Issue Feb 3, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@Akagi201

Akagi201 commented Feb 3, 2016

I wrote a redis http interface using the redis and pool sub-package.

In the main function

p, err := pool.New("tcp", u.Host, concurrency) // concurrency = 3

In the function called in the main function

func redisDo(p *pool.Pool, cmd string, args ...interface{}) (reply *redis.Resp, err error) {
    conn, err := p.Get()
    errHndlr(err)
    defer p.Put(conn)

    // do the request.
    reply = conn.Cmd(cmd, args...)
    if err = reply.Err; err != nil {
        if err != io.EOF {
            Fatal.Println("redis", cmd, args, "err is", err)
        }
        return
    }

    return
}

My redis-server has a timeout config timeout 300.

Here is my problem: When the program is idle for more than 300 seconds, I will get an EOF error returned from the redis. That is because the redis-server close the connection, but I can't get notified when I use conn, err := p.Get().

So, what is the right way to use radix's pool package?

For now, I just retry 3 times to establish the connection again when I get EOF error. I hope the redis pool can deal the redis-server timeout silently without my concerns.

Could you give me some suggestions?

@mediocregopher

This comment has been minimized.

Show comment
Hide comment
@mediocregopher

mediocregopher Feb 3, 2016

Owner

Hi there! Unfortunately radix's pool package does not spin up any go-routines of its own to do any work in the background, so there's no way for it to keep connections alive on its own. A very simply method you can use to ensure connections don't timeout would be to have something like this called after you create the pool:

go func() {
    for {
        p.Cmd("PING")
        time.Sleep(1 * time.Second)
    }
}()

There's no need to check for errors in there or anything, the Cmd method on pool will handle any connection errors it sees and make sure that dead connections don't get returned to the pool.

On that note, your code could be made a little simpler if you yourself used the Cmd method on pool, so you wouldn't have to worry about the Get and Put. That's not really related to your problem, but maybe it'll save you some lines :P

Owner

mediocregopher commented Feb 3, 2016

Hi there! Unfortunately radix's pool package does not spin up any go-routines of its own to do any work in the background, so there's no way for it to keep connections alive on its own. A very simply method you can use to ensure connections don't timeout would be to have something like this called after you create the pool:

go func() {
    for {
        p.Cmd("PING")
        time.Sleep(1 * time.Second)
    }
}()

There's no need to check for errors in there or anything, the Cmd method on pool will handle any connection errors it sees and make sure that dead connections don't get returned to the pool.

On that note, your code could be made a little simpler if you yourself used the Cmd method on pool, so you wouldn't have to worry about the Get and Put. That's not really related to your problem, but maybe it'll save you some lines :P

@Akagi201

This comment has been minimized.

Show comment
Hide comment
@Akagi201

Akagi201 Feb 4, 2016

@mediocregopher Thanks for your answers very much!

Akagi201 commented Feb 4, 2016

@mediocregopher Thanks for your answers very much!

@Akagi201 Akagi201 closed this Feb 4, 2016

@mediocregopher

This comment has been minimized.

Show comment
Hide comment
@mediocregopher

mediocregopher Feb 4, 2016

Owner

No problem! Let me know if there's anything else I can help with

On Wed, Feb 3, 2016, 7:14 PM Bob Liu notifications@github.com wrote:

@mediocregopher https://github.com/mediocregopher Thanks for your
answers very much!


Reply to this email directly or view it on GitHub
#21 (comment)
.

Owner

mediocregopher commented Feb 4, 2016

No problem! Let me know if there's anything else I can help with

On Wed, Feb 3, 2016, 7:14 PM Bob Liu notifications@github.com wrote:

@mediocregopher https://github.com/mediocregopher Thanks for your
answers very much!


Reply to this email directly or view it on GitHub
#21 (comment)
.

@Akagi201

This comment has been minimized.

Show comment
Hide comment
@Akagi201

Akagi201 Feb 4, 2016

@mediocregopher What I may be worried is whether the period "PING" request cost a lot for the redis server. And I tested that your code also works for redis-server restart.

Akagi201 commented Feb 4, 2016

@mediocregopher What I may be worried is whether the period "PING" request cost a lot for the redis server. And I tested that your code also works for redis-server restart.

@mediocregopher

This comment has been minimized.

Show comment
Hide comment
@mediocregopher

mediocregopher Feb 4, 2016

Owner

Its only getting one every second, if that sleep wasn't there I think it
wouldn't be great, but with it I can't imagine it being a problem. I've hit
a redis server on meh hardware with thousands of actual requests per
second, a couple extra pings here and there won't hurt anything.

On Wed, Feb 3, 2016, 8:05 PM Bob Liu notifications@github.com wrote:

@mediocregopher https://github.com/mediocregopher What I may be worried
is whether the period "PING" request cost a lot for the redis server. And I
tested that your code also works for redis-server restart.


Reply to this email directly or view it on GitHub
#21 (comment)
.

Owner

mediocregopher commented Feb 4, 2016

Its only getting one every second, if that sleep wasn't there I think it
wouldn't be great, but with it I can't imagine it being a problem. I've hit
a redis server on meh hardware with thousands of actual requests per
second, a couple extra pings here and there won't hurt anything.

On Wed, Feb 3, 2016, 8:05 PM Bob Liu notifications@github.com wrote:

@mediocregopher https://github.com/mediocregopher What I may be worried
is whether the period "PING" request cost a lot for the redis server. And I
tested that your code also works for redis-server restart.


Reply to this email directly or view it on GitHub
#21 (comment)
.

@Akagi201

This comment has been minimized.

Show comment
Hide comment

Akagi201 commented Feb 4, 2016

@mediocregopher Thanks~~

@daemonfire300

This comment has been minimized.

Show comment
Hide comment
@daemonfire300

daemonfire300 Feb 19, 2016

@mediocregopher I think, this should probably be handled like in redigo or other libraries, where the pool checks if the connection is alive, if it throws an error == not alive and spins up a new connection and returns it.

@mediocregopher I think, this should probably be handled like in redigo or other libraries, where the pool checks if the connection is alive, if it throws an error == not alive and spins up a new connection and returns it.

@mediocregopher

This comment has been minimized.

Show comment
Hide comment
@mediocregopher

mediocregopher Feb 19, 2016

Owner

@daemonfire300 that solution doesn't actually improve user experience in any way, although it feels like it would. The problem is that in between the time of the pool giving you the connection and you using it there's a chance of that connection dying, regardless of if the pool is doing its own checks or not. So your application code must expect that the connection might be dead when it goes to use it, and handle that appropriately, in order to be correct. So even if radix's pool did do its own checking your code would still look the same.

The one difference is that sometimes you end up needing to write ping loops like above. I have an idea about how to go about making them not be needed in most cases that also doesn't involve an extra go-routine, I just haven't had a chance to work on it yet.

Owner

mediocregopher commented Feb 19, 2016

@daemonfire300 that solution doesn't actually improve user experience in any way, although it feels like it would. The problem is that in between the time of the pool giving you the connection and you using it there's a chance of that connection dying, regardless of if the pool is doing its own checks or not. So your application code must expect that the connection might be dead when it goes to use it, and handle that appropriately, in order to be correct. So even if radix's pool did do its own checking your code would still look the same.

The one difference is that sometimes you end up needing to write ping loops like above. I have an idea about how to go about making them not be needed in most cases that also doesn't involve an extra go-routine, I just haven't had a chance to work on it yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment