2.x: Strawman proposal for uncaught exception configuration #5569
Conversation
This is a strawman example/proposal for configurable uncaught exception handling in RxJavaPlugins. This is in ref to ReactiveX#5234. The two options are to defer to current thread's handler (the current implementation and remains the default) and an optional `throw` option that wraps the throwable into an `UncaughtRxJavaException` for developers to watch for separately (and rethrowing, which requires some sort of `RuntimeException` wrapping).
ZacSweers
changed the title
ZacSweers
changed the title
Codecov Report
@@ Coverage Diff @@
## 2.x #5569 +/- ##
============================================
- Coverage 96.23% 96.19% -0.04%
- Complexity 5823 5825 +2
============================================
Files 631 632 +1
Lines 41421 41426 +5
Branches 5739 5740 +1
============================================
- Hits 39861 39851 -10
- Misses 616 625 +9
- Partials 944 950 +6
Continue to review full report at Codecov.
|
|
There is a way without changing the plugins: public class UncaughtCrash {
@Before
public void before() {
RxJavaPlugins.setErrorHandler(e -> {
Thread.currentThread().setUncaughtExceptionHandler((t, f) -> {
Thread.currentThread().setUncaughtExceptionHandler(null);
throw (InternalError)f;
});
throw new InternalError(e);
});
}
@After
public void after() {
RxJavaPlugins.setErrorHandler(null);
}
@Test
public void test1() {
RxJavaPlugins.onError(new IOException());
}
@Test
public void test2() {
RxJavaPlugins.onError(new IOException());
}
}This will bypass the |
|
That's not a solution for application use though, which could interfere with crash reporting tools |
How? You can restore the previous handler: or RxJavaPlugins.setErrorHandler(e -> {
UncaughtExceptionHandler saved = Thread.currentThread().getUncaughtExceptionHandler();
Thread.currentThread().setUncaughtExceptionHandler((t, f) -> {
saved.uncaughtException(t, f);
throw (InternalError)f;
});
throw new InternalError(e);
}); |
|
The first solution will get stuck in a cycle of try/catching before eventually bombing out in The second doesn't actually avoid the issue for an android environment because sending it to the saved handler first will just exit the process before the decorator has a chance to handle it in a catch clause again. |
|
Could you provide an example runnable test that demonstrates the way you want to catch these exceptions?
/cc @JakeWharton |
|
Can do. I'll be away from my computer tomorrow but will circle back here with it Monday |
|
Here's a demo. https://github.com/hzsweers/rxuncaughtissue Just click the button to see, result will be a long trace like this: |
|
I see. Unless there is a free pass for the exception thrown, this PR would end up with the same complicated cause chain. So if I understand correctly, you want the following two:
The first case, when inside an unit test, could be simulated via the Otherwise and in option 2, the question is what would you even do about that undeliverable error? Either way, you'd still have to get a free pass for that type of crash by modifying And finally, even if such behavior override would be added, I'd go for a static volatile flag instead of a system parameter so it can be enabled and disabled when testing the feature itself. |
|
The first case can definitely be handled in tests, but I'm basically trying to write a tool to help with crash reporting by decorating the observer with this observer that, in the event of the delegate observer throwing an What modifications would need to go into throwIfFatal? Even if it's something as simple as letting OnErrorNotImplemented bubble up, it'd go a long way to detecting unhandled errors and being able to tie them to the point of subscription. Volatile flag sounds fine to me |
Another
Not a good idea as it is an established exception type and unexpected new behavior of it.
I'd think you are using the wrong hook then; Other option I see would be a new hook |
|
I think a key part of this is that it probably shouldn't be tied to Not really sure how to proceed on this though :/ |
|
Actually, I think just knowing state about LambdaObserver would be suitable. It's probably not an unsafe assumption that if someone is actually implementing onError, they are responsible for their own implementation. |
|
Any further thoughts on exposing LambdaObserver state? |
|
Modifying |
|
Cool. I can start working on a PR. With regards to it being internal currently, would you want to make it public with |
|
Implementing an interface simplifies the detection of the feature but otherwise will make a method public on I'd say let's add a |
|
Sounds good, I'll start on that |
Followup from ReactiveX#5569, and allows you to introspect if the resulting observer has missing error consumption and subsequently supplies a default (throwing) one.
|
Continued in #5590 |
* Implement HasDefaultErrorConsumer Followup from #5569, and allows you to introspect if the resulting observer has missing error consumption and subsequently supplies a default (throwing) one. * Add `@since` * Add tests * Add support in relevant completable observers * Add support in ConsumerSingleObserver * Add support in MaybeCallbackObserverTest * Add support in LambdaSubscriber * Switch to CompositeObserver and onErrorImplemented() * Update wording to use Introspection * Update tests and flip implementation logic to match naming
This is a strawman example/proposal for configurable uncaught exception handling in RxJavaPlugins. This is in ref to #5234.
The two options are to defer to current thread's handler (the current implementation and remains the default) and an optional
throwoption that wraps the throwable into anUncaughtRxJavaExceptionfor developers to watch for separately (and rethrowing, which requires some sort ofRuntimeExceptionwrapping).If the proposal is agreeable/after API design is in place, I can add tests.