EVAL with error inside transaction doesn't throw exception

[GoogleCodeExporter]

Hi,

I've encountered an issue when executing a script inside a transaction.
When the script return an error (redis.error_reply), the transaction just 
returns "OK" as if nothing went wrong.

What steps will reproduce the problem?

var conn = GetConnection();

bool result;
using(var tx = conn.CreateTransaction())
{
      tx.Scripting.Eval(0, "return redis.error_reply('something went wrong')", new string[0], new object[0]);
      result = conn.Wait(tx.Execute());
}

Assert.IsFalse(result); //--> This one is true

I would expect that the exception is simply rethrown by tx.Execute(). Right 
now, nothing happens and the Execute method return "True".

I've been able to fix the issue by making some simple changes:

1. I've updated class MultiRedisResult. You can find the updated code in the 
attachment.
2. I've made ExecMessage non-critical by removing the call to Critical() in 
it's constructor.

This way, the exception is thrown by the Execute method when the script returns 
an error.

Regards
Gert

Original issue reported on code.google.com by g...@stylelabs.com on 25 Apr 2013 at 1:53

Attachments:

• MultiRedisResult.cs

[GoogleCodeExporter]

It is incorrect to expect that to throw an error. That transaction is actually 
committed. The fact that one of the commands reported failure does not impact 
the transaction. For example, try (in redis-cli):

multi
set abc 123
eval "return redis.error_reply('oops')" 0
set def 456
exec

you should see:

1) OK
2) (error) oops
3) OK

and if you then check:

get abc
"123"
get def
"456"

To check whether your *script* completed, you should capture the return value 
*of the `Eval` command*, and then **after** the `Execute`, see what it says.

I haven't looked at the changes yet - I will do; but: to clarify: it is 
entirely correct to report that as a completed transaction - because *it is 
one*.

Original comment by marc.gravell on 25 Apr 2013 at 3:48

[GoogleCodeExporter]

Hi Marc,

Thank you for your quick reply.

You're right about the transaction completing successfully. The changes don't 
change the output of the Execute command. It'll still be true.

The only thing it changes is that an exception is thrown by the execute method, 
just like the Eval method does.

I agree that it might not be the most appropriate way to handle this situation. 
I guess the alternative would be that the Eval method returns some kind of 
error object representing the lua error table. Right now it doesn't return 
anything (object[0]) when something goes wrong.

Anyway, the changes allow the following:

bool result;
try
{
     result = await tx.Execute();
}
catch(RedisException)
{
    ...
}
Assert.IsTrue(result); --> will still be true

Gert

Original comment by g...@stylelabs.com on 25 Apr 2013 at 4:05

[GoogleCodeExporter]

If the script error is not reported, that is a bug and will be fixed. I'm 
somewhat reluctant on making that an exception, though - that may make it hard 
to see what actually happened. It seems to me that the right thing to do there 
is to let the user check the result (when fixed) of the eval.

However, I would be willing to have a second version of Execute{some different 
name/signature} that indicates an AggregateException if any of the individual 
operations reported failure.

Original comment by marc.gravell on 25 Apr 2013 at 4:49

[GoogleCodeExporter]

To clarify I mean that it is the transaction that (without changing the 
name/sig) that should not report an exception - the transaction was perfectly 
happy.

Original comment by marc.gravell on 25 Apr 2013 at 4:57

[GoogleCodeExporter]

I've double-checked, and it looks like Eval is already picking up the error 
correctly, both inside and outside of a transaction, so the code *that exists* 
is working fine. What you propose has some merit, though - perhaps a second 
form of Execute that returns  an enum making the result clear:

- TransactionAborted // precondition failed or "WATCH" data changed
- CompletedWithoutErrors // completed, no operations reported errors 
- CompletedWithErrors // completed, but one or more operations reported errors

I genuinely do not think that it is the right thing for the transaction 
*itself* to throw here; the transaction is *not* reporting failure (if the 
transaction *does* report a failure, it would already throw).

Thoughts?

Original comment by marc.gravell on 26 Apr 2013 at 8:40

[GoogleCodeExporter]

redis.error_reply() does not throw an exception. It simply returns a structure 
containing an error message. No scripts are stopped because of it.

return redis.error_reply() does not stop the script because of an exception. It 
stops the script by completing it. I don't see any errors thrown. 

If an exception is to be thrown, in my humble opinion, it's at the call site of 
Eval() by checking the return value of the script to see if an error object was 
returned.

Original comment by d...@stylelabs.com on 26 Apr 2013 at 9:04

[GoogleCodeExporter]

which is exactly what I've done; it works correctly. Please see the 2 new tests 
shown here: 
https://code.google.com/p/booksleeve/source/diff?spec=svn3ebf71ee1d03aec2f32cab2
4eb57c17141abf91d&r=3ebf71ee1d03aec2f32cab24eb57c17141abf91d&format=side&path=/T
ests/Scripting.cs

Don't forget that *all* operations in BookSleeve are async; you don't know the 
result until you check the status of the Task, which only happens when a 
response comes back. You'll see that the task does indeed become "faulted", 
with the correct exception that represents the message returned by the script.

If you aren't seeing any errors thrown, that is because you aren't checking the 
outcomes of your tasks: well, that is normal. That is what *always* happens if 
you neglect the outcomes of tasks.

If I'm missing something, then my apologies - but maybe you could provide an 
example that shows the error not being correctly propagated, noting that 
"because I didn't check (via wait, await, result, etc)" is *not* a valid 
example.

Original comment by marc.gravell on 26 Apr 2013 at 9:18

[GoogleCodeExporter]

to clarify: I didn't change any implementation for those tests; they simply 
validated that it was already working

Original comment by marc.gravell on 26 Apr 2013 at 9:21

[GoogleCodeExporter]

I've updated my code to check the result of the eval task in a try-catch 
structure. This works fine the way it is right now.

I didn't realize that the eval task would return a value once 
transaction.Execute was called. My mistake :).

By the way, I think it's fine that waiting for the result of the eval operation 
throws an exception. There's no need to implement it any other way. The redis 
result of that operation starts with "-", so it is in fact an error.

You can close this issue.

And thanks for the great library!

Gert

Original comment by g...@stylelabs.com on 26 Apr 2013 at 9:32

[GoogleCodeExporter]

That's the fun thing about redis transactions... results are "QUEUED" until the 
EXEC, at which point you get them all in a multi-bulk - so yes, once you've 
executed the transaction the results of all the individual operations becomes 
available. You can `await` them etc if that is the most convenient thing.

Original comment by marc.gravell on 26 Apr 2013 at 11:07

• Changed state: Invalid