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

Fix multiple exception handler invocation in Android #3056

Merged
merged 1 commit into from Dec 6, 2021

Conversation

1zaman
Copy link
Contributor

@1zaman 1zaman commented Dec 1, 2021

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.

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.
@qwwdfsad qwwdfsad self-requested a review December 6, 2021 09:55
Copy link
Member

@qwwdfsad qwwdfsad left a 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

@1zaman
Copy link
Contributor Author

1zaman commented Dec 7, 2021

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 dispatchUncaughtException() method, which does invoke the pre-handler, even if a custom uncaught exception handler was set (which was the whole point behind introducing the pre-handler in the first place).

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.

woainikk pushed a commit that referenced this pull request Dec 14, 2021
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.
yorickhenning pushed a commit to yorickhenning/kotlinx.coroutines that referenced this pull request Jan 28, 2022
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.
pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
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.
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.

None yet

2 participants