Improving error/exception handling

[yeplato]

We are using your redis-dart in our server. Really appreciate your work!

We suggest that the error/exception handling may be improved a little bit.
In short, when we do try{ await cmd.send_object( )} or try{ pubSub.subscribe()}, we may expect that we can easily capture all underlying errors/exceptions (no unhandled exception to crash the server). However, we found that the only way to capture all errors/exceptions are adding runZoned() to wrapper conn.connect() or PubSub(cmd), which makes the upper-layer logic not every easy to organize.
Maybe registering stream error handlers from original socket across all underlying streams (e.g. lazystream) are a possible solution for this improvement?

The code snippets below are explaining the issue in details.

import 'dart:async';
import 'package:redis/redis.dart';
​
var local_host = '127.0.0.1';
var cmd_list = ['set', 'somekey', 'somevalue'];
​
main() async {
  RedisConnection conn;
  Command cmd;
​
  //runZoned() help to capture exceptions for cmd.send_object()
  await runZoned(() async {
    conn = RedisConnection();
    cmd = await conn.connect(local_host, 6379);
  }, onError: (error) {
    print('>>>>> captured by runZoned() wrapping RedisConnection.connect, error:$error');
  });
​
  while (true) {
    await Future.delayed(Duration(seconds: 2));
    //try{} cannot capture all exceptions here without using runZoned() above
    //changing to use runZoned() here cannot capture all exceptions
    try {
      var result = await cmd.send_object(cmd_list);
      print(result);
    } catch (error) {
      print('>>>>> captured by try{} wrapping Command.send_object, error:$error');
    }
  }
}

/*
* experiment steps
1.start a local redis server on terminal.
2.start this testing program.
3.then, kill the local redis server on terminal
4.saw exceptions are captured from multiple places

The output of this experiment
>>>>> captured by try{} wrapping Command.send_object, error:stream closed
>>>>> captured by runZoned() wrapping RedisConnection.connect, error:SocketException: OS Error: Connection reset by peer, errno = 54, address = 127.0.0.1, port = 54249
* */

import 'dart:async';
import 'package:redis/redis.dart';
​
var local_host = '127.0.0.1';
var cmd_list = ['set', 'somekey', 'somevalue'];
​
main() async {
  RedisConnection conn;
  Command cmd;
  PubSub pubSub;
​
  //runZoned() help to capture exceptions for pubSub.subscribe()
  await runZoned(() async {
    conn = RedisConnection();
    cmd = await conn.connect(local_host, 6379);
  }, onError: (error) {
    print('>>>>> captured by runZoned() wrapping RedisConnection.connect, error:$error');
  });
​
  //runZoned() help to capture exceptions for pubSub.subscribe()
  await runZoned(() async {
    pubSub = await PubSub(cmd);
  }, onError: (error) {
    print('>>>>> captured by runZoned() wrapping PubSub, error:$error');
  });
​
  while (true) {
    await Future.delayed(Duration(seconds: 2));
    //try{} cannot capture all exceptions here without using runZoned() above
    //changing to use runZoned() here cannot capture all exceptions
    try {
      pubSub.subscribe(['ch1', 'ch2', 'ch3']);
    } catch (error) {
      print('>>>>> captured by try{} wrapping PubSub.subscribe, error:$error');
    }
  }
}
​
/*
experiment steps
1.start a local redis server on terminal.
2.start this testing program.
3.then, kill the local redis server on terminal
4.saw exceptions are captured from multiple places

The output of this experiment
>>>>> captured by runZoned() wrapping PubSub, error:stream is closed
>>>>> captured by runZoned() wrapping RedisConnection.connect, error:SocketException: OS Error: Connection reset by peer, errno = 54, address = 127.0.0.1, port = 54256
* */

[ra1u]

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.

[yeplato]

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
  });

[rajmaniar]

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.

[ra1u]

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.

[rajmaniar]

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.

[ra1u]

@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.

[ra1u]

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