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

Coroutines do not log uncaught exceptions in Android #148

Closed
wardellbagby opened this Issue Oct 18, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@wardellbagby
Copy link

wardellbagby commented Oct 18, 2017

https://github.com/wardellbagby/KotlinExceptions

I created a sample app to show what I mean.

When a coroutine has an uncaught exception, it's manually calling the Thread UncaughtExceptionHandler. A consequence of that is Android's uncaught exception pre-handler that handles logging isn't invoked.

The README of the linked project goes more in depth.

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Oct 18, 2017

You correctly describe the current behavior of kotlinx.coroutines. By default, on Android they use the default Android policy on uncaught exception handling.

What is your question or suggestion?

@wardellbagby

This comment has been minimized.

Copy link
Author

wardellbagby commented Oct 18, 2017

My suggestion is that, for Android, I believe it's more beneficial to have the uncaught exception pre-handler be invoked as well as the Thread's UncaughtExceptionHandler. I imagine most developers use the information posted to the log by the pre-handler for debugging issues, and with the way coroutines are now on Android, uncaught exceptions will crash the app without actually posting the logs.

While it is possible for developers to manually fix this, I believe it would be better for coroutines, on Android, to either rethrow the exception so the system will log it, or reimplement Android's pre-handler so that the exception is easier to track down.

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Oct 19, 2017

@wardellbagby How this can be implemented, given the constraint that kotlinx-coroutines-core can depend only on Java 6 APIs? How this "pre-handler" can be invoked?

We can provide some ready-to-use implementation as a part of kotlinx-coroutines-androind module, but, frankly, I cannot find any mention of "pre-handler" in Android APIs either. Can you give a link?

@wardellbagby

This comment has been minimized.

Copy link
Author

wardellbagby commented Oct 19, 2017

The pre-handler concept is completely hidden by the Android system, and it can't be invoked directly. You can take a look at where it's set by RuntimeInit and where the methods are in the Thread class in which it's calling:

RuntimeInit: https://github.com/aosp-mirror/platform_frameworks_base/blob/2efbc7239f419c931784acf98960ed6abc38c3f2/core/java/com/android/internal/os/RuntimeInit.java#L142

Thread: https://android.googlesource.com/platform/libcore/+/oreo-release/ojluni/src/main/java/java/lang/Thread.java#1893 and https://android.googlesource.com/platform/libcore/+/oreo-release/ojluni/src/main/java/java/lang/Thread.java#1943

I think in a perfect world, the default coroutine contexts (e.g., CommonPool, Unconfined, and others) would include a coroutine exception handler by default that either logs the exception, or rethrows it so the Android system will catch it and make sure the pre-handler is called. Or there would be a new coroutine context included in the kotlinx-coroutines-android module that would be CommonPool with a default coroutine exception handler that would be recommended for use.

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Oct 20, 2017

@wardellbagby Would you suggest as to how this can be implement? How do we know what logging framework to use given there are so many different logging frameworks out there?

Actually, the variety of logging frameworks was the chief reason to support CoroutineExceptionHandler so that you can log they way you want it in your app. That is also the reason why the default exception handler uses getUncaughtExceptionHandler and not simply dumps exception to console or does something else of that nature. You can install whatever default uncaught exception handler you want in your application, and it will be used by all the default contexts automatically.

@wardellbagby

This comment has been minimized.

Copy link
Author

wardellbagby commented Oct 20, 2017

Hmm. That's a good point; I didn't think much of the different logging frameworks.

I think in this case, and only this case specifically where the Thread's uncaught exception will be invoked on Android, it's okay to just dump it straight to console via Android's android.util.Log. Since it would be using Android specific functionality, this would likely be best off inside of kotlinx-coroutines-android as a new coroutine with a CoroutineExceptionHandler, since it would only be giving it the same functionality other exceptions get when caught by the system.

However, the downsides would be that developers would have to manually specify that coroutine context and might still be confused if they aren't aware of it. So while it's a solution, it'll have similar pitfalls to what is currently happening.

I would say it could be made so that the DefaultDispatcher is changed only on Android, but it doesn't look like there's any platform specific code in core (which is a great thing) and this isn't major enough to warrant the introduction of that, since this isn't technically incorrect behavior.

In light of all of this information, I'm changing my mind. The best solution might just be to add this quirk into the documentation so that fellow developers are aware of it. There are at least two ways to fix this developer side, and neither of them are difficult to implement. For the sake of any future web crawlers who come across this looking for a code sample, I'll provide the two I'm aware of.

  1. Create a new UncaughtExceptionHandler (best if placed in #onCreate inside of the developer's Application subclass) that logs the issue before passing it on to the old UncaughtExceptionHandler
val currentUncaughtExceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
Thread.setDefaultUncaughtExceptionHandler({ thread, exception ->
            Log.println(Log.ERROR, thread.name, Log.getStackTraceString(exception))
            currentUncaughtExceptionHandler.uncaughtException(thread, exception)
        })
  1. Create a CoroutineExceptionHandler that either logs it directly (safe) or rethrows the exception so the system can catch it (unsafe)
val handler = CoroutineExceptionHandler { _, ex ->
        Log.println(Log.ERROR, LOG_TAG, Log.getStackTraceString(ex))
    }

or

val handler = CoroutineExceptionHandler { _, ex ->
        throw ex
    }
launch(CommonPool + handler) {
        //your code here
}
@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Oct 20, 2017

An alternative solution is to include an Android-specific hack into kotlinx.coroutines. We can use reflection to see if there is a static getUncaughtExceptionPreHandler function on the Thread class and if there is one, then retrieve the corresponding handler and use it, just like Android does it.

It can be made safer by using services framework to register additional exception handler, with kotlinx-coroutines-android module providing that service on Android. This way, we can choose to avoid reflection altogether, but just log it mimicking the logic of Android's default uncaught exception logger that is set as pre-handler.

@wardellbagby

This comment has been minimized.

Copy link
Author

wardellbagby commented Oct 20, 2017

If you believe that's a good idea, I'm definitely 100% for it!

@elizarov elizarov added the enhancement label Nov 6, 2017

@elizarov elizarov changed the title Coroutines not logging uncaught exceptions in Android Coroutines do not log uncaught exceptions in Android Nov 14, 2017

@elizarov elizarov closed this in d166eb6 Dec 4, 2017

@curiousily

This comment has been minimized.

Copy link

curiousily commented Dec 5, 2017

getUncaughtExceptionPreHandler seems to be missing from the Thread class (targeting above Android 5). A snippet showing how to use this would be great!

@saied89

This comment has been minimized.

Copy link
Contributor

saied89 commented Nov 7, 2018

I've just spent hours debugging a simple issue. Whats the current best practice for getting uncaught exceptions on android?

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Nov 7, 2018

@saied89 Do you have kotlinx-coroutines-android module added to your dependencies?

@saied89

This comment has been minimized.

Copy link
Contributor

saied89 commented Nov 7, 2018

@elizarov
Yeah. Adding that fixed it. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.