Enable the configuration of the uncaught exception handler

[realdadfish]

There are some use cases where a global uncaught exception handler should not be used, e.g. in library projects. With this PR it is possible to disable the setup of that handler by doing

LogPersister.installUncaughtExceptionHandler(false)

By default, the handler is still installed, though.

[coveralls]

Coverage increased (+0.2%) to 29.799% when pulling 68951b6 on realdeadfish:make-ueh-install-configurable into 8757f2a on ibm-bluemix-mobile-services:master.

[svkrish]

First, apologies for the delayed response and thanks for the PR. Can you please clarify the following...

if (!install && (current instanceof UncaughtExceptionHandler)) {
Thread.UncaughtExceptionHandler original = ((UncaughtExceptionHandler) current).getOriginal();
Thread.setDefaultUncaughtExceptionHandler(original);
}

This means our SDK's UncaughtExceptionHandler will not be called and we will lose out on reporting crashes.

In the present implementation we anyway call the Thread's default exception handler after logging crash data - why is that not ok - what is it that will break.

[realdadfish]

First, apologies for the delayed response and thanks for the PR.

No worries :)

Can you please clarify the following...
if (!install && (current instanceof UncaughtExceptionHandler)) {
Thread.UncaughtExceptionHandler original = ((UncaughtExceptionHandler) current).getOriginal();
Thread.setDefaultUncaughtExceptionHandler(original);
}

This means our SDK's UncaughtExceptionHandler will not be called and we will lose out on reporting crashes.

My rationale for this was simply to have a method to cleanly "reset" to the previous state. If I install the uncaught exception handler, the previously set exception handler is not lost, but stored in our custom handler (and called eventually). Now, if I decide to uninstall it again, any previous exception handler is reinstated, so the situation is identical to what it was before I installed it in first place.

In the present implementation we anyway call the Thread's default exception handler after logging crash data - why is that not ok - what is it that will break.

It was mostly a matter of cleaning things up. Otherwise a call to LogPersister.installUncaughtExceptionHandler(false) would have made little sense / could not have worked at all. Since we're talking about static APIs here everywhere it would be also hard from an API user perspective to figure out when / where exactly he would disable the custom handler, since he's unaware of the inner workings / defaults of the library.

[svkrish]

• I agree that the original handler is lost when 'unsetContext' is called. We can fix that by setting it back as the default in 'unsetContext'
• I don't prefer allowing this install and uninstall for two reasons:
  i) this is an internal detail that the crashlogs feature rides on and need not be visible to the user
  ii) want to avoid the side effect that an uninstall will have which is our SDK losing the capability to log crashes

So in summary, we will call the original as we do today after logging crashes - so no behaviour coded in the originial is lost. And in unsetConext we will resurrect the default handler to the original one. ??

[realdadfish]

Well, since all you have there is static API at different levels, which make it hard to follow what is initialized when and where (especially from a caller's perspective), I tend to disagree as an uninstall functionality would at least be orthogonal to an install functionality, but it's your project. So I'll make the appropriate changes.

[realdadfish]

@svkrish Please have another 👀

[tobilarscheid]

any updates on this?

[svkrish]

Here is what I had commented last ...

"So in summary, we will call the original as we do today after logging crashes - so no behaviour coded in the originial is lost. And in unsetConext we will resurrect the default handler to the original one. ??"

We will have the above fix in place next week.

If you are looking at merging the PR, well that is what we may want to do. While the LogPersister is public we expect most application developers to use only the Analytics and Logger abstractions. If you really want to have the LogPersister the way it is suggested in the PR I suppose you could write your own LogPersister implementing the https://github.com/ibm-bluemix-mobile-services/mfp-clientsdk-android-analyticsspec/blob/master/lib/src/main/java/com/ibm/mobilefirstplatform/clientsdk/android/logger/internal/LogPersisterInterface.java and you could encapsulate the suggested functionality in that and eventually set that to the Logger using the setLogPersister method.