Store error at output-level when using NestedMultiOutput #328

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@jongyeol
Contributor
jongyeol commented Aug 4, 2016 edited

Make sure that:

  • You have read the contribution guidelines.

  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.

  • You submit test cases (unit or integration tests) that back your changes.

When an exception occured in evalsha(), it will be thrown. But, if I use evalsha() with MULTI return type, the exception didn't occured and it was included in result. This behavior is strange for users.

Case 1: Normal case.

This case use evalsha with ScriptOutputType.STATUS return type.

@Test
public void lettuceLuaUnknownSha1TestWithStatus() throws InterruptedException {
    RedisClient lettuce = RedisClient.create("redis://localhost:6379");
    RedisAsyncCommands<String, String> client = lettuce.connect().async();
    RedisFuture<String> results = client.evalsha("unknown-sha1", ScriptOutputType.STATUS, "test1");
    results.whenComplete((res, ex) -> {
        logger.info("results=" + res);
        logger.info("ex=" + ex);
    });
    Thread.sleep(10000);
}

Output: normal case. results was null, and the exception was delivered into ex.

results=null
ex=com.lambdaworks.redis.RedisCommandExecutionException: NOSCRIPT No matching script. Please use EVAL.

Case 2: Strange case

This case use evalsha with ScriptOutputType.MULTI return type.

Code:

@Test
public void lettuceLuaUnknownSha1WithMulti() throws InterruptedException {
    RedisClient lettuce = RedisClient.create("redis://localhost:6379");
    RedisAsyncCommands<String, String> client = lettuce.connect().async();
    RedisFuture<List<Object>> results = client.evalsha("unknown-sha1", ScriptOutputType.MULTI, "test1");
    results.whenComplete((res, ex) -> {
        logger.info("results=" + res);
        logger.info("ex=" + ex);
    });
    Thread.sleep(10000);
}

Output: ex was null. And, the exception was included in results. It looks a bug.

results=[com.lambdaworks.redis.RedisCommandExecutionException: NOSCRIPT No matching script. Please use EVAL.]
ex=null
@coveralls
coveralls commented Aug 4, 2016 edited

Coverage Status

Coverage increased (+0.2%) to 92.918% when pulling 7248c1d on jongyeol:feature/fix-evalsha-with-multi into 860e200 on mp911de:master.

@jongyeol jongyeol changed the title from Fix to occur an exception when calling evalsha with multi return type to The exception was not occured in evalsha when I use MULTI return type Aug 9, 2016
@jongyeol
Contributor
jongyeol commented Aug 9, 2016

@mp911de , Could you review this too?

@mp911de
Owner
mp911de commented Aug 10, 2016 edited

What's the reason for using the MULTI output? The intention behind MULTI is to collect transaction results and dispatch these to the previously issued commands.

@mp911de mp911de commented on an outdated diff Aug 10, 2016
.../com/lambdaworks/redis/ReactiveCommandDispatcher.java
@@ -104,6 +104,12 @@ public void complete() {
super.complete();
if (getOutput() != null) {
+ if (getOutput().hasError()) {
@mp911de
mp911de Aug 10, 2016 Owner

Let's keep that block where it was before. The idea is to emit items which arrived before an error occurred and then terminate with a call to onError.

Streaming output will still behave that way but non-streaming outputs would no longer emit items but terminate directly.

@mp911de mp911de commented on an outdated diff Aug 10, 2016
...a/com/lambdaworks/redis/output/NestedMultiOutput.java
@@ -40,7 +40,8 @@ public void set(ByteBuffer bytes) {
@Override
public void setError(ByteBuffer error) {
- output.add(new RedisCommandExecutionException(decodeAscii(error)));
+ super.setError(error);
+ output.add(new RedisCommandExecutionException(this.error));
@mp911de
mp911de Aug 10, 2016 Owner

I also think that adding errors to the output is an error. I'd propose to remove the method override fall back to the default error propagation.

@mp911de
Owner
mp911de commented Aug 10, 2016

The reason why the reactive test fails is because the proxy/invocation handler is not propagating the exception. I fixed the invocation handler on master with 0cb10c4 so you might want to rebase your PR.

@mp911de mp911de added the bug label Aug 10, 2016
@mp911de mp911de added this to the Lettuce 4.2.2 milestone Aug 10, 2016
@jongyeol jongyeol Fix to occur an exception when calling evalsha with multi return type
8dc511e
@jongyeol
Contributor

@mp911de , I rebased and updated this PR. 😄

@coveralls

Coverage Status

Coverage increased (+0.1%) to 92.938% when pulling 8dc511e on jongyeol:feature/fix-evalsha-with-multi into f43d678 on mp911de:master.

@coveralls

Coverage Status

Coverage increased (+0.1%) to 92.938% when pulling 8dc511e on jongyeol:feature/fix-evalsha-with-multi into f43d678 on mp911de:master.

@mp911de mp911de added a commit that referenced this pull request Aug 11, 2016
@jongyeol @mp911de jongyeol + Store error at output-level when using NestedMultiOutput #328
NestedMultiOutput now stores errors in the command output and not inside the output content to propagate errors correctly.
bb6d78c
@mp911de mp911de added a commit that referenced this pull request Aug 11, 2016
@jongyeol @mp911de jongyeol + Store error at output-level when using NestedMultiOutput #328
NestedMultiOutput now stores errors in the command output and not inside the output content to propagate errors correctly.
99a6cb0
@mp911de mp911de added a commit that referenced this pull request Aug 11, 2016
@jongyeol @mp911de jongyeol + Store error at output-level when using NestedMultiOutput #328
NestedMultiOutput now stores errors in the command output and not inside the output content to propagate errors correctly.
94026bf
@mp911de mp911de added a commit that referenced this pull request Aug 11, 2016
@mp911de Polishing
Move test for NestedMultiOutput into own class.

Original pull request: #328.
4810d76
@mp911de mp911de added a commit that referenced this pull request Aug 11, 2016
@jongyeol @mp911de jongyeol + Store error at output-level when using NestedMultiOutput #328
NestedMultiOutput now stores errors in the command output and not inside the output content to propagate errors correctly.
4e318a8
@mp911de mp911de added a commit that referenced this pull request Aug 11, 2016
@mp911de Polishing
Move test for NestedMultiOutput into own class.

Original pull request: #328.
2259a32
@mp911de
Owner
mp911de commented Aug 11, 2016

Thanks a lot. That's merged and backported.

@mp911de mp911de closed this Aug 11, 2016
@jongyeol jongyeol deleted the jongyeol:feature/fix-evalsha-with-multi branch Aug 11, 2016
@mp911de mp911de changed the title from The exception was not occured in evalsha when I use MULTI return type to Store error at output-level when using NestedMultiOutput Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment