-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix multiple exception handler invocation in Android #3056
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
Conversation
The AndroidExceptionPreHandler was changed in commit 5ea9339 to not invoke the uncaught exception pre-handler on Android Pie and above, and to instead invoke the thread's uncaught exception handler directly on those versions, as Android Pie had added logic in the default uncaught exception handler to ensure that the pre-handler would be invoked. However, the uncaught exception handler is already being invoked for all unhandled coroutine exceptions, so this change in the AndroidExceptionPreHandler caused the uncaught exception handler to be invoked twice in Android Pie and above. Since the default uncaught exception handler implementation in Android kills the app immediately, this normally wouldn't make a difference. However, if an app sets a custom uncaught exception handler (either at the thread or global level) that doesn't always kill the app, then the duplicate invocations might cause undesired or wrong behaviour. This is now fixed by removing the uncaught exception handler invocation from AndroidExceptionPreHandler. Since the pre-handler was introduced in Android Oreo, I have also added a minimum SDK check for Android Oreo as well. Also edited the documentation in the comments to add more details and context.
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.
Thanks!
For the record, verified this fix on AS simulators with various API versions
Note that if a custom uncaught exception handler is set that doesn't call through to the default handler, then the pre-handler (and logging) would still be bypassed, since the additional checking in Android Pie was done only on the default handler implementation. As opposed to standard non-coroutine exceptions that are handled via the private I think this is fine, since it works in the default use-case (without a custom exception handler), and it's probably preferable to avoid using unsupported API as much as possible, especially considering the warnings added in Android Pie, as shown in #822. Most Android apps will not bypass the default uncaught exception handler, and those that do can just add logging themselves if they're handling uncaught coroutine exceptions. Though it would potentially result in double logging if the exception can be thrown from outside of a coroutine context as well (and thus trigger the pre-handler), but that probably isn't a big deal. Long-term, the special handling for Android can probably be removed completely, considering that it only affects Android Oreo (which is somewhat old at this point), and only developers for the most part (as it only impacts local logging). Those people who still use Android Oreo devices for development after a certain point can be asked to add the patch locally. |
The AndroidExceptionPreHandler was changed in commit 5ea9339 to not invoke the uncaught exception pre-handler on Android Pie and above, and to instead invoke the thread's uncaught exception handler directly on those versions, as Android Pie had added logic in the default uncaught exception handler to ensure that the pre-handler would be invoked. However, the uncaught exception handler is already being invoked for all unhandled coroutine exceptions, so this change in the AndroidExceptionPreHandler caused the uncaught exception handler to be invoked twice in Android Pie and above. Since the default uncaught exception handler implementation in Android kills the app immediately, this normally wouldn't make a difference. However, if an app sets a custom uncaught exception handler (either at the thread or global level) that doesn't always kill the app, then the duplicate invocations might cause undesired or wrong behaviour. This is now fixed by removing the uncaught exception handler invocation from AndroidExceptionPreHandler. Since the pre-handler was introduced in Android Oreo, I have also added a minimum SDK check for Android Oreo as well. Also edited the documentation in the comments to add more details and context.
The AndroidExceptionPreHandler was changed in commit 5ea9339 to not invoke the uncaught exception pre-handler on Android Pie and above, and to instead invoke the thread's uncaught exception handler directly on those versions, as Android Pie had added logic in the default uncaught exception handler to ensure that the pre-handler would be invoked. However, the uncaught exception handler is already being invoked for all unhandled coroutine exceptions, so this change in the AndroidExceptionPreHandler caused the uncaught exception handler to be invoked twice in Android Pie and above. Since the default uncaught exception handler implementation in Android kills the app immediately, this normally wouldn't make a difference. However, if an app sets a custom uncaught exception handler (either at the thread or global level) that doesn't always kill the app, then the duplicate invocations might cause undesired or wrong behaviour. This is now fixed by removing the uncaught exception handler invocation from AndroidExceptionPreHandler. Since the pre-handler was introduced in Android Oreo, I have also added a minimum SDK check for Android Oreo as well. Also edited the documentation in the comments to add more details and context.
The AndroidExceptionPreHandler was changed in commit 5ea9339 to not invoke the uncaught exception pre-handler on Android Pie and above, and to instead invoke the thread's uncaught exception handler directly on those versions, as Android Pie had added logic in the default uncaught exception handler to ensure that the pre-handler would be invoked. However, the uncaught exception handler is already being invoked for all unhandled coroutine exceptions, so this change in the AndroidExceptionPreHandler caused the uncaught exception handler to be invoked twice in Android Pie and above. Since the default uncaught exception handler implementation in Android kills the app immediately, this normally wouldn't make a difference. However, if an app sets a custom uncaught exception handler (either at the thread or global level) that doesn't always kill the app, then the duplicate invocations might cause undesired or wrong behaviour. This is now fixed by removing the uncaught exception handler invocation from AndroidExceptionPreHandler. Since the pre-handler was introduced in Android Oreo, I have also added a minimum SDK check for Android Oreo as well. Also edited the documentation in the comments to add more details and context.
The
AndroidExceptionPreHandler
was changed in #1030 to not invoke the uncaught exception pre-handler on Android Pie and above, and to instead invoke the thread's uncaught exception handler directly on those versions, as Android Pie had added logic in the default uncaught exception handler to ensure that the pre-handler would be invoked.However, the uncaught exception handler is already being invoked for all unhandled coroutine exceptions, so this change in the
AndroidExceptionPreHandler
caused the uncaught exception handler to be invoked twice in Android Pie and above.Since the default uncaught exception handler implementation in Android kills the app immediately, this normally wouldn't make a difference. However, if an app sets a custom uncaught exception handler (either at the thread or global level) that doesn't always kill the app, then the duplicate invocations might cause undesired or wrong behaviour.
This is now fixed by removing the uncaught exception handler invocation from
AndroidExceptionPreHandler
.Since the pre-handler was introduced in Android Oreo, I have also added a minimum SDK check for Android Oreo as well. Also edited the documentation in the comments to add more details and context.