Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle undeliverable errors in rxObservable #2178

Closed

Conversation

1zaman
Copy link
Contributor

@1zaman 1zaman commented Aug 1, 2020

If an error can't be delivered to the observer due to the observer already having disposed the subscription, then it should be reported to the RX global exception handler. This was already being done in the implementations for rxSingle, rxMaybe, and rxCompletable, but not in rxObservable.

Fixes one inconsistency from #2173. I'll look into fixing the other as well.

1zaman added 2 commits July 30, 2020 00:31
Ensure that the RX error handler actually gets invoked
If an error can't be delivered to the observer due to the observer
already having disposed the subscription, then it should be reported
to the RX global exception handler. This was already being done in
the implementations for rxSingle, rxMaybe, and rxCompletable, but
not in rxObservable.

Copied the test over from the other implementations as well.

Also removed the previous test in ObservableTest, which actually was
testing rxSingle instead of rxObservable, and was too complicated
and dependent on race conditions (both rxSingle and rxObservable now
have a simple and deterministic test for undeliverable error
handling). It was also not well implemented, and I originally tried
to fix it to be more robust in commit 58ad0cd.

Fixes Kotlin#2173.
Comment on lines -167 to +168
* To make behaviour consistent and least surprising, we always handle fatal exceptions
* by coroutines machinery, anyway, they should not be present in regular program flow,
* thus our goal here is just to expose it as soon as possible.
* To make behaviour consistent and least surprising, we always deliver fatal exceptions to the
* RX global exception handler.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was changed in #1638, but seems like the comment was not updated.

Comment on lines -171 to +170
subscriber.tryOnError(cause)
if (!handled && cause.isFatal()) {
if (!subscriber.tryOnError(cause) || (!handled && cause.isFatal())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the handled flag is redundant here after the earlier check for CancellationException, since it could only be handled in the current implementation if it was a CancellationException. There doesn't seem to be any special handling of fatal exceptions by the coroutine. However, I have left that logic as it was here.

Comment on lines -141 to -161
@Test
fun testExceptionAfterCancellation() {
// Test that no exceptions were reported to the global EH (it will fail the test if so)
val handler = { e: Throwable ->
assertFalse(e is CancellationException)
}
withExceptionHandler(handler) {
RxJavaPlugins.setErrorHandler {
require(it !is CancellationException)
}
Observable
.interval(1, TimeUnit.MILLISECONDS)
.take(1000)
.switchMapSingle {
rxSingle {
timeBomb().await()
}
}
.blockingSubscribe({}, {})
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems to have been taken from the issue reported at #252. However it's complicated and not well implemented (I originally tried to improve it in commit 58ad0cd). Also, it was actually testing rxSingle instead of rxObservable, which is why it didn't catch the issue in the first place that this pull request is trying to solve. rxSingle (and the other RX builders) already have a simpler and deterministic test for undeliverable error handling (testUnhandledException()), which I have copied over in the ObservableExceptionHandlingTest test suite, so I deleted this test since should be no further need for it.

@1zaman 1zaman marked this pull request as draft August 1, 2020 21:31
@elizarov elizarov requested a review from qwwdfsad September 10, 2020 15:14
@qwwdfsad
Copy link
Collaborator

Fixed it in 1.5.0, thanks for the report

@qwwdfsad qwwdfsad closed this May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants