Improving error/exception handling about redis-dart HOT 7 CLOSED

I guess it has to do with my lack of understanding on how to properly handle errors. https://dart.dev/guides/libraries/futures-error-handling.
It is also something that holds back #11

It is now clear, that we need to improve in this direction.
First step would be writing few unit tests.

from redis-dart.

I'll chime in here with some unsolicited observations. :)

I've only looked through this code briefly so I might be totally off base but here's what I noticed:

Starting here: https://github.com/ra1u/redis-dart/blob/master/lib/pubsub.dart#L15

The Future.doWhile and the _senddummy are future stacks without any error/exception handling. The _senddummy in particular will only complete completer with a success value from RedisParser.parseredisresponse. So if RedisParser.parseredisresponse throws that'll bubble up to the Future.doWhile but never complete completer. I think adding error path through that future stack that ultimately closes the PubSub._stream_controller or passes an error to client listener should solve it...and it should be pretty straight forward to add.

from redis-dart.

Thank you for the quick reply!

One more tiny example to mention. For pubSub.listen() scenario, it would be nicer to capture the errors (e.g. Redis connection breaks) with onDone or onError. I made an example below.

  await runZoned(() async {
    pubSub = await PubSub(cmd);
  }, onError: (error) {
    print('>>>>> captured by runZoned wrapping PubSub'); //still print here
  });

  runZoned(() {
    pubSub.getStream().listen((message) {
      print(message);
    }, onDone: () {
      print('onDone');  //not print here
    }, onError: (error) {
      print('onError: $error'); //not print here
    }, cancelOnError: true);
  }, onError: (error) {
    print('>>>>> captured by runZoned onError: $error');  //not print here
  });

from redis-dart.

What is expected behavior if
RedisParser.parseredisresponse throws in PubSub?

Current naive implementations, throws this up to connection opener, so it has consequence of dropped connection.
Are we able to correctly recover trough same connection, so that we reuse all futures if we catch this error earlier?

What to do if redis server sends response we are unable to receive for some reason? Do we propagate error and then resume receiving further requests, like nothing happened?

I think there are two kinds of issues.
One is user generated exceptions, that he wants to propagate trough redis-dart stack and other is parser/connection generated errors.

When user exception throws (like DatabaseReceivedValueIsTooLargeTo Process or UserCredentialsInvalid) we need to correctly propagate this trough this stack, and this is something that is possible to recover, since connection and protocol is unaffected.

RedisConnection/protocol errors that happens less frequently is something that is probably impossible to recover from and best we can do is to notify that non recoverable error occurred.

Dart has similar approach using Errors

These are not errors that a caller should expect or catch - if they occur, the program is erroneous, and terminating the program may be the safest response

and Exception

It is intended to be caught, and it should contain useful data fields.

So maybe we should use Errors in our code to distinguish them from user kind of Exceptions.
When RedisError happens we terminate Connection and user is discouraged from catching them, and user exceptions needs to somehow propagate our redis-dart twisted callback stack.

from redis-dart.

I think the above makes A LOT of sense!
In our application we have a clearly typed Exceptions and Errors that extend the dart base classes for just this reason.

I think throwing an exception or error at the connection opener or constructor isn't a great since it's hard to catch those (like you'd have to wrap a constructor in a new zone to trap an error).

You could provide an error call back in the constructor but as a consumer I'd rather get my exception or error at the point of use, then my application can take the appropriate action then.

The commit above looks good, it doesn't include a fix for the OP's pubsub issue specifically but it could be easily extended address that. My only comment would be it would be nice if the RedisError and TransactionError implemented the dart:core exception or error interface; that'd be a bit more friendly.

from redis-dart.

@yeplato can you rerun test against recent master branch and report how it behaves regarding this reported issue. I am looking forward to close this.

from redis-dart.

Fixed in version 3.1.0. Please do test and DO REPORT issue if you still find one.
Reference example in https://github.com/ra1u/redis-dart/#pubsub

from redis-dart.