-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Avoid swallowing errors in Completable #3733
Conversation
What happened to |
@@ -1835,6 +1835,8 @@ public void onCompleted() { | |||
public void onError(Throwable e) { | |||
ERROR_HANDLER.handleError(e); | |||
mad.unsubscribe(); | |||
Thread thread = Thread.currentThread(); | |||
thread.getUncaughtExceptionHandler().uncaughtException(thread, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just inline Thread.currentThread()
call here and below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or create a private method perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, two usages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private method is fair. done.
@akarnokd sorry, IJ formatted it. Will clean up and push again. |
7309342
to
d22ba1d
Compare
👍 |
@@ -1927,7 +1933,12 @@ public void onSubscribe(Subscription d) { | |||
|
|||
return mad; | |||
} | |||
|
|||
|
|||
private static void deliverUncaughtException(Throwable e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it's a good candidate to live in Exceptions
😸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require it to be public, sadly, and this isn't an API that RxJava needs to provide for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we can create ExceptionsInternal
under internal
package, but yeah… k, let's extract it if somewhere else same functionality will be needed.
just a nit about |
Instead, deliver them up to the thread's uncaught exception handler. Fixes ReactiveX#3726
d22ba1d
to
8b55303
Compare
removed timeouts |
👍 |
Avoid swallowing errors in Completable
Instead, deliver them up to the thread's uncaught exception handler.
Fixes #3726