google / agera

Reactive Programming for Android
Apache License 2.0
7.2k stars 639 forks source link

[Question] Some problem about error handling #39

Closed zjutkz closed 8 years ago

zjutkz commented 8 years ago

Hi,sorry to bother you guys again. I know we can use Result & Attempt to do a better error handling in agera. But I found a problem is agera is weak of handling unchecked exception,like RuntimeExeception. When I try this:

private Supplier<Result<Integer>> safeStrSupplier = new Supplier<Result<Integer>>() {
        @NonNull
        @Override
        public Result<Integer> get() {

            throw new RuntimeException("exception!!!");
        }
    };
safeRepository = Repositories.repositoryWithInitialValue(Result.<Integer>absent())
                .observe()
                .onUpdatesPerLoop()
                .attemptGetFrom(safeStrSupplier).orSkip()
                .....

The work flow will not skip but throw the exception.It means we cannot catch the exception unless we use try-catch block,does it?

ghost commented 8 years ago

You are correct, the functional interfaces cannot throw exceptions, not even a RuntimeException. Instead it needs to be wrapped in Result. The code above would instead look like

public Result<Integer> get() {
  return Result.failure(new RuntimeException("exception!!!"));
}

When using attemptXYZ + orSkip the Repository will continue to contain the old value (in your example absent(). If you want to explicitly handle an error from a functional interface you could either not use attemptXYZ and pass the Result to the next functional interface (might be tedious) or alternatively use .orEnd. This will allow you to recover from your failure if you want.

private Supplier<Result<Integer>> safeStrSupplier = new Supplier<Result<Integer>>() {
  @NonNull
  @Override
   public Result<Integer> get() {
     return Result.failure(new RuntimeException("exception!!!"));
   }
};
safeRepository = Repositories.repositoryWithInitialValue(Result.<Integer>absent())
  .observe()
  .onUpdatesPerLoop()
  .attemptGetFrom(safeStrSupplier).orEnd(new Function<Throwable, Result<Integer>>() {
    @NonNull
    @Override
    public Result<Integer> apply(@NonNull Throwable error) {
      // check the throwable and do possible explicit error handling here, or recover
      return Result.present(3);
    }
  })
  .....

Just as a reference, the Result class and this way of doing error handling was heavily inspired by Scala's Try and Option classes. There's a description of the Scala Try class that I like here; The Neophyte's Guide to Scala Part 6: Error Handling With Try. But of course, the examples are written in Scala.

zjutkz commented 8 years ago

@ernstsson Thanks for your responding,I know the code you show me will work correctly.But if agera cannot handle unchecked exception by itself,it will trouble developers like this:

private Supplier<Result<Integer>> safeStrSupplier = new Supplier<Result<Integer>>() {
        @NonNull
        @Override
        public Result<Integer> get() {

            Integer[] errorArray = new Integer[]{1};

            return Result.present(errorArray[1]);
        }
    };
 safeRepository = Repositories.repositoryWithInitialValue(Result.<Integer>absent())
                .observe()
                .onUpdatesPerLoop()
                .attemptGetFrom(safeStrSupplier).orSkip()
                .....

Developers may make a mistake like the code I shown above,an ArrayIndexOutOfBoundsException. When they wrote the code,they had no idea about this situation,they just thought they were right,and the whole program will failed because of this exception. I know RxJava can handle this problem because all unchecked exceptions like ArrayIndexOutOfBoundsException will be caught and send to onError(Throwable e) method.So I wonder if agera can do this job like what Rxjava does.

ghost commented 8 years ago

I see your point. The idea of Result is to handle expected exceptions, runtime or not. For unexpected exceptions (NullPointerException, ArrayIndexOutOfBoundsException etc.) we've always had the philosophy to crash, find, and fix before any lib client release. Even though soaking up all errors in onError, expected as well as unexpected, doesn't mesh well with my personal philosophy I recognise the need for exactly this for systems with a different error handling style.

Also as you say, attemptXYZ won't help, since this as with Result only takes care of the expected exceptions. I do think it'd be possible to add another optional operation to the compiled repositories. Something like .onUnexpectedExeption(Function<Throwable, T> errorRecovery) where T is the type of the value the Repository contains. There's a few kinks to work out with this (such as; should we guarantee what thread this is to be called on, etc) but I hope it should be fairly straightforward to add.

Actually, now that I think about it, it could be quite useful for systems written with the "crash, find, fix" philosophy too, since the errorRecovery function above could easily crash i debug and "gracefully fail" in release. Might have some fun refactoring to do of our app soon :)

Ping to @maxtroy too, who usually have thoughts about these things.

zjutkz commented 8 years ago

@ernstsson @maxtroy Thanks a lot,I think I should reconsider about expected and unexpected exception,lol.