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

Fix discarding of error values #323

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

woodsbury
Copy link

Fixes #313

When discarding the responses from previous commands that weren't
successfully read (e.g. if their context had a deadline that was
exceeded), RESP error values get treated the same as connection errors
and cause any reading from the connection to stop. This can cause a
response to not be read off of the connection or marked for discarding,
and therefore leave the connection out of sync.

This fix disables error bubbling during the discarding process to
prevent these error values from interfering.

@mediocregopher
Copy link
Owner

Hi @woodsbury , thanks for the PR! The solution you've given here was used as part of unmarshalAgg, but I don't think it is applicable to this case. It happens to work for your test case because you used strings as your into variables, and so when this line:

err = resp3.Unmarshal(c.br, discardMU.unmarshalInto, o)

is called it happens to be able to unmarshal the SimpleError into the string (because they can be coerced). But if you make this change:

diff --git a/conn_test.go b/conn_test.go
index 73abce0..b65318c 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -290,7 +290,7 @@ func TestConnDeadlineExceeded(t *T) {
                                        }

                                        innerCtx, cancel := context.WithTimeout(ctx, mkTimeout(timeout))
-                                       into := ""
+                                       into := 0
                                        err := conn.Do(innerCtx, Cmd(&into, "EVAL", "return redis.error_reply('NOTOK')", "0"))
                                        cancel()

you'll see the test breaks again.

You're original solution that you posted in the issue, however, works fine even with that into := 0, and I think is the right way to solve this problem. If you could use that solution in this PR instead I think we can merge this.

Thanks again for debugging this and putting together a test case, that's made reviewing this much easier 🙏

@woodsbury
Copy link
Author

No problem, I've updated my changes to check the error type instead.

When discarding the responses from previous commands that weren't
successfully read (e.g. if their context had a deadline that was
exceeded), RESP error values get treated the same as connection errors
and cause any reading from the connection to stop. This can cause a
response to not be read off of the connection or marked for discarding,
and therefore leave the connection out of sync.

This fix changes the discarding process to ignore RESP error values.
@mediocregopher mediocregopher merged commit 248804f into mediocregopher:v4 Jul 14, 2022
@mediocregopher
Copy link
Owner

Looks good, thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants