Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DoWithCancel #351

Open
garyburd opened this issue Jun 27, 2018 · 1 comment
Open

Add DoWithCancel #351

garyburd opened this issue Jun 27, 2018 · 1 comment

Comments

@garyburd
Copy link
Member

garyburd commented Jun 27, 2018

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.

@stevenh
Copy link
Collaborator

stevenh commented Jul 21, 2019

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?

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

No branches or pull requests

2 participants