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

Avoid swallowing errors in Completable #3733

Merged
merged 1 commit into from
Mar 2, 2016

Conversation

loganj
Copy link
Contributor

@loganj loganj commented Feb 24, 2016

Instead, deliver them up to the thread's uncaught exception handler.

Fixes #3726

@akarnokd
Copy link
Member

What happened to CompletableTest?

@@ -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);
Copy link
Contributor

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?

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, two usages.

Copy link
Contributor Author

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.

@loganj
Copy link
Contributor Author

loganj commented Feb 24, 2016

@akarnokd sorry, IJ formatted it. Will clean up and push again.

@loganj loganj force-pushed the completable-errors branch 2 times, most recently from 7309342 to d22ba1d Compare February 24, 2016 21:39
@akarnokd
Copy link
Member

👍

@@ -1927,7 +1933,12 @@ public void onSubscribe(Subscription d) {

return mad;
}


private static void deliverUncaughtException(Throwable e) {
Copy link
Contributor

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 😸

Copy link
Contributor

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.

Copy link
Contributor

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.

@artem-zinnatullin
Copy link
Contributor

just a nit about timeout in tests, otherwise 👍

Instead, deliver them up to the thread's uncaught exception handler.

Fixes ReactiveX#3726
@loganj
Copy link
Contributor Author

loganj commented Feb 25, 2016

removed timeouts

@stevegury
Copy link
Member

👍

stevegury added a commit that referenced this pull request Mar 2, 2016
Avoid swallowing errors in Completable
@stevegury stevegury merged commit 423172f into ReactiveX:1.x Mar 2, 2016
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.

5 participants