MULTI..EXEC transactions and asynchronous Call commands #10

Open
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants

Hi,

This pull request addresses three issues:

  1. When a MULTI..EXEC transaction fails because of a WATCH condition, an error is returned by ReadAll. It is difficult to test this error to see if it was due to an aborted transaction or something else. I have named the error as ErrMultiAborted, so now you can just do a check like so:

    if err == redis.ErrMultiAborted { // transaction aborted, try again

  2. The Call method of an AsyncClient cannot return any errors, so I removed the err return value. It calls Write in bytes.Buffer, but the docs for that method explicitly say it will never return an err (it just has the return type to match the io.Writer interface). Removing this lets careful people know it is okay to assume the call will succeed.

  3. When using WATCH with MULTI..EXEC transactions, the normal pattern is:

    WATCH foo
    GET foo
    MULTI
    do stuff using the value of foo that is now guaranteed to not change
    EXEC

AsyncClient handles the MULTI..EXEC part great, but it is a bit verbose to run the commands that happen before the MULTI is issued. This adds a SyncCall to AsyncClient that works more like Call from Client. It fails if there are already commands in the queue.

Thanks,

Russ

@@ -83,7 +87,7 @@ func (r *Reply) parseMultiBulk(buf *bufin.Reader, res []byte) {
l, _ := strconv.Atoi(string(res))
if l == -1 {
- r.Err = errors.New("-MULTI-BULK: nil reply")
+ r.Err = ErrMultiAborted
@simonz05

simonz05 May 29, 2012

Owner

-1 in a Multi-Bulk does not always mean that the watch condition was not met. It can also mean that the key you ask for does not exist[1].

[1] http://redis.io/topics/protocol#nil-reply

@russross

russross May 30, 2012

I'm new to redis, so my apologies if I'm getting this all wrong. The documentation you referenced seems to indicate a nil reply means an empty list for a "bulk" section, not a "multi-bulk" section. In a "multi-bulk" section the nil reply means a timeout (for a blocking list action) or an aborted "multi...exec" block (my use case).

In either case, it does not invalidate the results of the other actions in the same queued sequence of instructions. Perhaps this error should be stored for the individual command ("exec" in my case) rather than returning an error for the entire sequence? I'm not sure I like that idea either, but I think the current API is lacking something.

What I need is some way to detect when a "multi...exec" block was aborted due to a "watch" condition being violated. I couldn't see any good way to detect that with the existing code. I could extract the text of the error, but that seems like a bad idea.

Any other suggestions?

@simonz05

simonz05 May 30, 2012

Owner

The documentation you referenced seems to indicate a nil reply means an empty list for a "bulk" section, not a "multi-bulk" section. In a "multi-bulk" section the nil reply means a timeout (for a blocking list action) or an aborted "multi...exec" block (my use case).

I stand corrected. I had another look. I was thinking of -1 in a mulit-bulk reply (so that would be a bulk reply). I think this would be OK, except that the error condition could happen in other circumstances as well. If we look a the example from the protocol specification;

C: BLPOP key 1
S: *-1

In this case the multi-bulik -1 is an indication of a timeout. If we change the name of the Err to something more generic, I'm OK with the the named Err.

-func (ac *AsyncClient) Call(args ...interface{}) (err error) {
- _, err = ac.buf.Write(format(args...))
+func (ac *AsyncClient) Call(args ...interface{}) {
+ // note: bytes.Buffer.Write never returns an error
@simonz05

simonz05 May 29, 2012

Owner

I like this change. It reminds me of an anecdote from a lisp-programmer who traced a parameter through a call-stack of 20 so routines, just to find out it was never used. The programmer proceeds to ask the author of the code, what it was for. Just to find it was put it in "just in case", as a reserve.

- return err
+}
+
+// Issue a synchronous call on an async connection.
@simonz05

simonz05 May 29, 2012

Owner

I don't like this change due to;

  1. I think this might be too special case to add a new function to the interface.
  2. The interface already allows you do this without the added function.

I do however think this change does point out something which is unnecessary cumbersome to do with the current interface. Do you have an example of real code where you need/use this feature.

@russross

russross May 30, 2012

I do have real code that needs this; that's why I introduced it. To implement a transaction with redis, I do something like this (error handling skipped for brevity):

ac.SyncCall("watch", "accountbalance:123")
reply, _ := ac.SyncCall("get", "accountbalance:123")
n := reply.Elem.Int64()
// process account info; let's say it needs $10 added
ac.Call("multi")
ac.Call("set", "accountbalance:123", n + 10)
ac.Call("exec")

I'm new to redis, but as far as I know that's the only way to ensure that the "set" call only succeeds if the key has not been modified since the "get" call. It is useless to do all of this using queued calls (ac.Call), because the result of the "get" call would not be available until after the "exec" call returned.

My use case is a quite a bit more complicated than that, but the need is the same.

@simonz05

simonz05 May 30, 2012

Owner

I'm new to redis, but as far as I know that's the only way to ensure that the "set" call only succeeds if the key has not been modified since the "get" call.

For the example you provided I think this used to be the only way to achieve that, but with the introduction of scripting you could do that with the EVAL command. So you might want to look into that if this is something you do allot. So not a solution the issue, but it might help your programming problem.

I'm however still hesitant about the semantics of this code. So I understand your problem, but I don't like the solution. I don't think there is anything wrong with the code. I just think it's messy to put a SyncCall on a AsyncClient. And if I pull this I know you depend on it, and it would be harder to remove, than to add at a later stage. I really appreciate the effort you put into the issue. I suggest you add a helper function until I figure out a good way to do this, or just maintain the small patch.

@russross

russross Jun 4, 2012

I'm looking into EVAL now, and I'll probably go with that option (saves several network roundtrips). It looks like redis 2.6 is getting close to release, so it seems reasonable to start depending on this.

Thanks,

Russ

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