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

Connection stuck in bad state after a Redis Error #48

Closed
rajmaniar opened this issue Sep 21, 2021 · 5 comments
Closed

Connection stuck in bad state after a Redis Error #48

rajmaniar opened this issue Sep 21, 2021 · 5 comments
Labels

Comments

@rajmaniar
Copy link

There appears to be a breaking bug where once the client returns a RedisError the Connection is permanently broken.

It's very easy to reproduce, just run the code below:

  var redis = RedisConnection();
  var cmd = await redis.connect("localhost", 6379);
  await cmd.send_object(["GARBAGE"]).catchError((e){
    print("***** Error GARBAGE -- $e");
  });

  await cmd.send_object(["get", "some_key",]).catchError((e){
    print("***** Error Get some_key -- $e");
  });

  await cmd.send_object(["get", "some_other_key",]).catchError((e){
    print("***** Error Get some_other_key -- $e");
  });

The Get commands fail with the same error as the GARBAGE command

It looks like the bug was introduced here:
3110c8e#diff-0169e92992be999e9a9de7482d45357ffeaa22f5a9f2adaa974fafd3c62ab661L41

The future chaining that's going on with _future I don't believe will work as expected since when the Future is completed with an error the successor will never be called.

@ra1u
Copy link
Owner

ra1u commented Sep 21, 2021

The future chaining that's going on with _future I don't believe will work as expected since when the Future is completed with an error the successor will never be called.

That is good point.

I believe that one possible fix is to wrap return future. We need to keep _future in connection.dart, allays without error and we yield error in future that gets to user.

@ra1u ra1u added the bug label Sep 21, 2021
@rajmaniar
Copy link
Author

I don't really understand what purpose _future serves but what you're suggesting sounds good to me.

@ra1u
Copy link
Owner

ra1u commented Sep 21, 2021

_future serves so that we can stack multiple request over same socket and then make repose, to appropriate caller when response is received.
It makes high performance example to work.

I made some modifications. @rajmaniar , can you run some test on your side and report if that improves this issue.

@rajmaniar
Copy link
Author

The changes seem to work well, thanks!

@ra1u
Copy link
Owner

ra1u commented Oct 20, 2021

fix released with v2.1.0

@ra1u ra1u closed this as completed Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants