JakeWharton / RxBinding

RxJava binding APIs for Android's UI widgets.
Apache License 2.0
9.68k stars 971 forks source link

verifyMainThread() checks in RxJava 2 variants don't go to onError #353

Closed ZacSweers closed 7 years ago

ZacSweers commented 7 years ago

Note sure if this is by design or not, but the exceptions thrown by verifyMainThread() fail hard rather than going to onError() (since it doesn't know the disposed state during the calling subscribe and will just bubble it straight to RxJavaPlugins#onError() or throw. Since we have the observer there, should we send the exception to its onError() instead?

Specifically, I think this is actually a significant behavior change from the implementation in RxJava 1 or using create(), which would send this to onError().

Result is if you want to gracefully handle that case, you end up having to try/catch the actual subscribe() call itself

OrdonTeam commented 7 years ago

I wouldn't like to change this behaviour. Calling such methods on wrong thread is developers' mistake and it should blow up in theirs face. I cannot imagine how you could handle such error in onError and how could you be prepared for it.

artem-zinnatullin commented 7 years ago

True ^, the only thing to keep in mind is that with RxJava 2 developers kinda have to add error Handler for RxJava plugins since all errors happened after unsubsuption go there (such as network request timeout) and there is a chance that exception from RxBinding will be silently swallowed by poorly written error hanlder and lead to hours spent in debugging.

ZacSweers commented 7 years ago

I agree it should blow up, but I think it should blow up by going to onError(). Idiomatic RxJava dictates as much, even on invalid inputs like null values. This just feels a big gotcha now.

ZacSweers commented 7 years ago

there is a chance that exception from RxBinding will be silently swallowed by poorly written error hanlder and lead to hours spent in debugging.

That argument doesn't really make sense as you could really claim it as a case against using any error handling in RxJava. The whole point is you have an elegant means of handling them.

artem-zinnatullin commented 7 years ago

The whole point is you have an elegant means of handling them

Which does not make sense for NotOnMainThreadException :)

If it'll be delivered to observer, plugin swallowing will not be an issue.

On Wed, Feb 1, 2017, 14:17 Zac Sweers notifications@github.com wrote:

there is a chance that exception from RxBinding will be silently swallowed by poorly written error hanlder and lead to hours spent in debugging.

That argument doesn't really make sense as you could really claim it as a case against using any error handling in RxJava. The whole point is you have an elegant means of handling them.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/JakeWharton/RxBinding/issues/353#issuecomment-276633059, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7B3G-VJnKU5dDLQGvb1UhR9dx8jGsgks5rYGnjgaJpZM4Ly3Ya .

ZacSweers commented 7 years ago

I disagree. I'm not sure what principle requires that thread exceptions must be hard crashes.

I am arguing it should be delivered to the observer. I'm not really sure what plugin swallowing to do with this. If the observer has no onError handling, it bubbles back to the plugin system anyway before finally throwing.

JakeWharton commented 7 years ago

It should go to onError.

ZacSweers commented 7 years ago

Cool, I'll work on a PR today