Skip to content
This repository has been archived by the owner on Sep 2, 2020. It is now read-only.

Monitor for unobserved errors #100

Merged
merged 1 commit into from
Jan 11, 2016

Conversation

stefanrusek
Copy link
Contributor

This defers the actual finalization handling to a secondary class.
Additionally if any of the continuations immediately observe the error,
then no additional finalization logic is performed.

Fixes #48

@richardjrossiii
Copy link
Member

I like the concept, and it seems like a fairly portable API to have everywhere. My one concern, however, is that finalize() is not guaranteed to be called on a user-controlled thread (in fact, on the desktop JVM, all finalizers are run on a dedicated thread), which makes the uncaught exception handler bit somewhat useless.

@stefanrusek
Copy link
Contributor Author

@richardjrossiii The fact that it runs on a GC thread doesn't make it useless. It might prevent that handler from directly calling UI methods, but it could still log the error someplace useful. Also the default handler could fire off a new Task to deal with the error like so:

final Executor myUIExecutor = ...;
Thread.setDefaultUncaughtExceptionHandler(new Thread.UncaughtExceptionHandler() {
    public void uncaughtException(Thread t, final Throwable e) {
        Task.Call(new Callable<Void>() {
            // anything you want to do..
        }, myUIExecutor);
    }
});

@grantland
Copy link
Member

This looks interesting! I want to play around with this to see how this plays out in different environments.

If it works well, we'd also want this configurable and off by default to not be a breaking change and so developers can turn it off in production environments.

@stefanrusek
Copy link
Contributor Author

To turn it off by default, we could just add a static field and getUnobservedExceptionHandler() and setUnobservedExceptionHandler() methods. the default would just eat the exception. Then use that instead of Thread.getDefaultUncaughExceptionHandler().

@facebook-github-bot
Copy link

@stefanrusek updated the pull request.

@stefanrusek
Copy link
Contributor Author

@grantland I made it configurable and added a test.

@grantland
Copy link
Member

FYI, I'll be without a computer until December and won't be able to take a look at this until then. Hope that's ok and sorry!

@stefanrusek
Copy link
Contributor Author

@grantland ping

@grantland
Copy link
Member

Finally got around to playing around with it and it's awesome! Thanks for updating it so that it doesn't throw on the uncaught exception handler by default.

@hallucinogen mind taking a look at the API and giving a second opinion?

ueh.unobservedException(faultedTask, new UnobservedTaskException(faultedTask.getError()));
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

one small edit: we should be using try { ... } finally { super.finalize() } here

@facebook-github-bot
Copy link

@stefanrusek updated the pull request.

* finalized. If it is finalized with a task, then the uncaught exception handler is exected
* with an UnobservedTaskException.
*/
public class UnobservedErrorNotifier {
Copy link
Member

Choose a reason for hiding this comment

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

we probably want to make this package private

Choose a reason for hiding this comment

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

+1

* @param t the task
* @param e the exception
*/
void unobservedException(Task<?> t, UnobservedTaskException e);

Choose a reason for hiding this comment

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

oh right. In Java isn't this usually in some form of onUnobservedException

Copy link
Member

Choose a reason for hiding this comment

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

This currently matches Thread.UncaughtExceptionHandler's pattern: https://developer.android.com/reference/java/lang/Thread.UncaughtExceptionHandler.html

Choose a reason for hiding this comment

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

lgtm then

@grantland
Copy link
Member

Last nit is to hide UnobservedErrorNotifier

@facebook-github-bot
Copy link

@stefanrusek updated the pull request.

This defers the actual finalization handling to a secondary class.
Additionally if any of the continuations immediately observe the error,
then no additional finalization logic is performed.

If no unobserved exception handler is set, then no finalization
processing takes place.

Fixes BoltsFramework#48
@stefanrusek
Copy link
Contributor Author

@grantland updated visibility

grantland added a commit that referenced this pull request Jan 11, 2016
@grantland grantland merged commit 114758f into BoltsFramework:master Jan 11, 2016
@grantland
Copy link
Member

Thanks for you're awesome contribution! As a thank you, wed like to send you some Parse swag. If you're interested, send me your mailing address and t-shirt size to my email address on my profile page.

@grantland grantland mentioned this pull request Jan 12, 2016
3 tasks
@grantland grantland modified the milestone: 1.4.0 Jan 12, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants