ra1u / redis-dart

fast redis protocol parser and client
MIT License
84 stars 35 forks source link

Connection stuck in bad state after a Redis Error #48

Closed rajmaniar closed 3 years ago

rajmaniar commented 3 years ago

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: https://github.com/ra1u/redis-dart/commit/3110c8eadb6b2b374ce2390f125110ad2869f58c#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 commented 3 years ago

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.

rajmaniar commented 3 years ago

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

ra1u commented 3 years ago

_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 commented 3 years ago

The changes seem to work well, thanks!

ra1u commented 3 years ago

fix released with v2.1.0