[Proposal] Add uncaught error hook

[ZacSweers]

From #5234

This is a proposal implementation for allowing configuration of how uncaught errors are handled. Currently they are handed directly to the current thread's UncaughtExceptionHandler, but this can have collateral effects in some scenarios. Namely:

1 - On Android, this handler will call System.exit(). There is no recourse, and no ability to try-catch. This is particularly dangerous for delegating observers that may wish to try/catch the delegate's lifecycle callbacks and safely degrade in the event of an exception.

2 - In JUnit, the opposite extreme happens: the exception is quietly discarded, allowing uncaught errors to go completely unnoticed in tests unless one installs an RxJavaPlugins errors hook.

This adds configuration at the "uncaught()" layer, which is different than what was initially proposed, but I feel the only way to safely control this behavior. Creating a custom onErrorNotImplemented consumer hook didn't work well in my testing because almost every other part of RxJava still try/catches these consumers with their own try/catch that then just re-routes to RxJavaPlugins.onError() anyway, thus preventing one from avoiding the default uncaught behavior that lies in there. By configuring it directly, we can control every facet that directly or indirectly routes through it.

This is effectly acting as a slightly more surgical hook than just using RxJavaPlugins.onError, the latter of which will still try/catch and fall back to uncaught behavior and effectly negates any plugin hooks that try to just rethrow exceptions through it. This also happens after the isBug() filtering layer.

[codecov]

Codecov Report

Merging #6378 into 2.x will decrease coverage by 0.03%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##                2.x    #6378      +/-   ##
============================================
- Coverage     98.25%   98.21%   -0.04%     
- Complexity     6292     6293       +1     
============================================
  Files           673      673              
  Lines         45092    45106      +14     
  Branches       6239     6240       +1     
============================================
- Hits          44306    44302       -4     
- Misses          248      264      +16     
- Partials        538      540       +2

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/io/reactivex/plugins/RxJavaPlugins.java	99.05% <83.33%> (-0.95%)	151 <6> (+5)
...ex/internal/operators/flowable/FlowableCreate.java	90% <0%> (-7.1%)	6% <0%> (ø)
...l/operators/observable/ObservableFlatMapMaybe.java	91.5% <0%> (-4.58%)	2% <0%> (ø)
.../io/reactivex/internal/schedulers/IoScheduler.java	89.36% <0%> (-4.26%)	9% <0%> (ø)
...nternal/operators/parallel/ParallelReduceFull.java	91.08% <0%> (-3.97%)	2% <0%> (ø)
...activex/internal/observers/QueueDrainObserver.java	97.43% <0%> (-2.57%)	21% <0%> (-1%)
...va/io/reactivex/internal/queue/SpscArrayQueue.java	97.61% <0%> (-2.39%)	22% <0%> (-1%)
...ava/io/reactivex/processors/BehaviorProcessor.java	96.86% <0%> (-2.25%)	60% <0%> (ø)
.../internal/disposables/ListCompositeDisposable.java	98% <0%> (-2%)	34% <0%> (-1%)
.../io/reactivex/disposables/CompositeDisposable.java	98.14% <0%> (-1.86%)	39% <0%> (-1%)
... and 33 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d40f923...ec1ae38. Read the comment docs.

[artem-zinnatullin]

Maybe reuse Java's UncaughtExceptionHandler instead of adding new interface to RxJava?

[ZacSweers]

Possibly. I'd be worried about confusion since it's kind of borrowing another pckage's API for something it wasn't intended for

[artem-zinnatullin]

Gramar

[artem-zinnatullin]

I'd deliver to current thread uncaught exception handler similarly how this method worked before

This change can also end up violating Reactive Streams in some cases

[ZacSweers]

The whole point of this is to avoid doing that though, but we have to try/catch this per the signature of the accept() method. I can avoid this by removing the signature, but not sure if that'd have any tradeoffs for consumers that just want to immediately rethrow.

How would it violate them? Note that this change is not the default behavior, which remains unchanged.

[akarnokd]

This change makes no sense. There is already a hook involved for RxJavaPlugins.onError, why add another one for when that hook crashes. Don't crash the onError hook - you are already in control of that.

[ZacSweers]

The goal is to enable crashing though, as the current behavior of just handing it to the nearest uncaught exception handler is what this wants to facilitate changing.

What about taking artem's suggestion in a different way and making this a custom hook to just provide the uncaughtexceptionhandler? I.e. - no consumer or anything, just allow specifying a custom handler and fall back to the current behavior of fetching it out of the current thread by default?

[akarnokd]

You are still trying to backdoor a backdoor. The overall issue sounds like you want to hook your developers who throw in their onError handler to get away with failures. What makes you think they wouldn't do it for this extra hook?

[ZacSweers]

onError will still funnel thrown exceptions through the current uncaught() implementation since it catches exceptions from any installed onError handler. It's not to get away with failures at all, but rather to

• in junit, actually fail tests when they have unhandled errors
• in Android - make them recoverable if this is just a delegating observer. The goal is to be able to try/catch without something calling system.exit() for us under the hood

Both require actually throwing the exception rather than silently passing it to an uncaught exception handler. I would be all for just using the onError handler API if it actually allowed for throwing exceptions that could be caught from outside the onError() method, but that's not possible at the moment

[akarnokd]

How about you install a custom hook to the current thread from the onError handler that will rethrow the original exception:

import java.io.IOException;
import java.lang.Thread.UncaughtExceptionHandler;

import org.junit.*;

import io.reactivex.Observable;
import io.reactivex.plugins.RxJavaPlugins;

public class HookThrowing {

    @Before
    public void before() {
        RxJavaPlugins.setErrorHandler(ex -> {
            UncaughtExceptionHandler h = Thread.currentThread().getUncaughtExceptionHandler();
            Thread.currentThread().setUncaughtExceptionHandler((t, e) -> {
                Thread.currentThread().setUncaughtExceptionHandler(h);
                HookThrowing.sneakyThrow(ex);
            });
            throw new RuntimeException("Fail up");
        });
    }
    
    @SuppressWarnings("unchecked")
    static <E extends Throwable> void sneakyThrow(Throwable ex) throws E {
        throw (E)ex;
    }
    
    @After
    public void after() {
        RxJavaPlugins.reset();
    }
    
    @Test
    public void test() {
        Observable.error(new IOException())
        .subscribe();
    }
}

[ZacSweers]

That feels a bit hacky, but I suppose it works. The "fail up" intermediate to rekick to the inner handler could be confusing since its trace still gets printed on the way to uncaught(), but just making that a sneaky throw site also could cover that.

This wouldn't be prone to threading concerns either since each handler handling is local to each thread right?

[akarnokd]

The repeatedly printed stacktrace is not relevant if you want to fail the test.

The uncaught handlers are thread-local and you only need to get past the catch around the onError handler. They are invoked together on the same thread.

[ZacSweers]

Haven't forgotten about this, just doing some further testing. While this works in a test, on an actual android app it seems to get stuck in a loop (bubbles up as a truncated compositeexception). Trying to dig in more to understand why the runtime is behaving different than the test

[ZacSweers]

Just backing up for a second - what was the reason again for why this couldn't be made configurable via system property to just throw rather than hand directly to the nearest uncaught exception handler? The only answer I could find on the past proposal was that exceptions couldn't be (safely?) thrown in a multithreaded environment.

Consider a trivial case in android:

Completable.complete()
    .subscribe(() -> throw new RuntimeException(""));

This will call System.exit() under the hood in the main thread. There's no opportunity to try/catch in that scenario. Obviously this example is a non-realistic, but now consider something a bit more realistic:

Completable.complete()
    .subscribe(() -> completionHandler());

void completionHandler() {
  // Some real code that inadvertently has a bug and throws an exception
}

This is a realistic case - bugs happen. For an operator with custom error tracking/logging though, such as a delegating one installed via RxJavaPlugins.setOnCompletableSubscribe, there's no way to safely attempt to call the delegate's onComplete() method even with a try/catch because of this behavior. This is on top of the inverse extreme of the UncaughtExceptionHandler discarding them silently like in JUnit.

[akarnokd]

Who is in control of the code that can fail? Your extra hook wouldn't work better than my handler juggling because RxJava may bounce sneaked errors over and over to the handler and eventually bubble out on some thread. Why can't you do what RxJava unit tests do and listen to all global errors and fail the test if any was reported?

[ZacSweers]

My point is that throws can be caught, but handing directly to a handler can't if that handler (like in many "main thread" contexts) just calls system.exit. Yes the junit case can be covered because it happens to have a mechanism for allowing a deferred throw later that doesn't get swallowed by rxjava's internals.

…

On Fri, Feb 1, 2019, 2:00 PM David Karnok ***@***.***> wrote: Who is in control of the code that can fail? Your extra hook wouldn't work better than my handler juggling because RxJava may bounce sneaked errors over and over to the handler and eventually bubble out on some thread. Why can't you do what RxJava unit tests do and listen to all global errors and fail the test if any was reported? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6378 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTEvoy2LYxrsVyxjnRK75uik9Hqffo7ks5vJLkSgaJpZM4aJ7u7> .

[ZacSweers]

I've managed to make this work. Planning to publish it somewhere separately. It's a bit wonky but I'm pretty sure this proposal isn't needed anymore