redigo icon indicating copy to clipboard operation
redigo copied to clipboard

Add DoWithCancel

Open garyburd opened this issue 7 years ago • 1 comments

Add DoWithCancel helper function after the Redis CLIENT UNBLOCK command is released.

// DoWithCancel executes cmd with args. If the context is canceled, then
// DoWithCancel unblocks c by executing the CLIENT UNBLOCK command using a
// connection returned from the get function.  Example:
//
//  result, err := redis.Values(redis.DoWithCancel(c, ctx, pool.Get, "BRPOP", "key", 0))
func DoWithCancel(c Conn, ctx context.Context, get func() Conn, cmd string, args ...interface{}) (interface{}, error) {
	id, err := redis.Int(redis.Do("CLIENT", "ID"))
	if err != nil {
		return err
	}
	stop := make(chan struct{})
	wait := make(chan struct{})

	go func() {
		defer close(wait)
		select {
		case <-stop:
		case <-ctx.Done():
                          // TODO: avoid deadlock. If argument c is from pool with Wait = true and the get argument
                         // is the Get() method from that same pool, then the following line can cause a deadlock.
			c := get()
			defer c.Close()
			_, err := c.Do("CLIENT", "UNBLOCK", id)
			// TODO: handle error, possibly by closing the network connection on argument c
		}
	}()

	r, err := c.Do(cmd, args...)
	close(stop)
	<-wait
	return r, err
}

To reduce extra roundtrips to the server, cache the result fo the CLIENT ID command in the low-level connection.

func (c *conn) DoWithTimeout(readTimeout time.Duration, cmd string, args ...interface{}) (interface{}, error) {
    ...
    isClientIDCmd := cmd == "CLIENT" && len(args) == 1 && args[0] == "ID" 
    if isClientIDCmd && c.clientID != nil {
        return c.clientID, nil
    }
    ...
    if isClientIDCmd && err == nil {
        c.clientID = reply
    }
    ...
}

EDIT: An alternative to caching the client ID in the low-level connection is to pipeline CLIENT ID with the application's command. This approach localizes the implementation to this one function and should have little impact on performance.

garyburd avatar Jun 27 '18 15:06 garyburd

One of the interesting challenges with this is knowing if unblock is supported, as allowing this command if not could result in unexpected results.

This could be handled with the error check and subsequent closing of connections but that's not obvious to the API consumer, as timeouts are usually an non-standard event it will take some identifying that this is the case.

A possible solution could be to have the pool be version aware.

The questions is given it's an edge case do could it cause a real issue for the server admin, and if not do we care?

stevenh avatar Jul 21 '19 10:07 stevenh