OnErrorFailedException fix

[konmik]

#2998

[akarnokd]

I guess by preventing the unwrapping, the error keeps propagating up, therefore, this change is essential. However, there could be code out there that depended on the original behavior (since Exceptions is part of the public API).

[konmik]

I don't think that there is such code. Such unwrapped exceptions were swallowed most of the time anyway.

[akarnokd]

Sounds reasonable.

[benjchristensen]

Interesting bug. I agree that removing the unwrapping is needed.

[akarnokd]

Looks good to me. 👍

[zsxwing]

Could you also fix the similar issues in Observable.subscribe and Observable.unsubscribe? Here are tests to reproduce them:

    @Test(expected = OnErrorFailedException.class)
    public void testOnErrorExceptionIsThrownFromSubscribe() {
        Observable.<Integer>create(s1 ->
                        Observable.<Integer>create(s2 -> {
                            throw new IllegalArgumentException("original exception");
                        }).subscribe(s1)
        ).subscribe(System.out::println, e -> {
            throw new RuntimeException();
        });
    }

    @Test(expected = OnErrorFailedException.class)
    public void testOnErrorExceptionIsThrownFromUnsafeSubscribe() {
        Observable.<Integer>create(s1 ->
                        Observable.<Integer>create(s2 -> {
                            throw new IllegalArgumentException("original exception");
                        }).unsafeSubscribe(s1)
        ).subscribe(System.out::println, e -> {
            throw new RuntimeException();
        });
    }

[konmik]

I would replace error handling in both of these methods with simple

    } catch (Throwable e) {
        Exceptions.throwIfFatal(e);
        try {
            subscriber.onError(hook.onSubscribeError(e));
        } catch (Throwable e2) {
            throw new OnErrorFailedException(e2);
        }
        return Subscriptions.unsubscribed();
    }

However, I think that the problem should be handled more generally.
The entire error handling policy should be reviewed and enforced.
Otherwise we will always see swallowed exceptions here and there.
Unfortunately, I don't have time right now to investigate the issue, I've just fixed what creates troubles for my current project.

[benjchristensen]

Thanks for digging in to help with this. I thought we had squashed all of these swallowing of errors! Apparently not :-(

[benjchristensen]

👍 Go ahead with this @akarnokd if you're still good with it.

[akarnokd]

It is good as it is. Merging. The thing @zsxwing asked for can be done in a separate PR.