Closed yeplato closed 2 years ago
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.
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
});
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.
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.
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.
@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.
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
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( )}
ortry{ 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 wrapperconn.connect()
orPubSub(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.