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

Replace "handle" with "tryHandle" for uncaught exceptions #3554

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

dkhalanskyjb
Copy link
Collaborator

This is a draft because it is a big binary-compatibility-breaking change. I was hoping to start a discussion about this so that we could evolve this to some form that's acceptable to merge.

I find this to be a much clearer mental model.

  • A handler can decide if it can process this particular exception.
  • A handler can be in an invalid state, and it can now pass handling the uncaught exception to the system, without making additional noise.
  • Before this change, there are two types of exception handlers:
    • Those that are used as coroutine context elements. If a coroutine context element processes an exception successfully, the system is not notified about them.
    • Those that are used as services. These are just notified about exception, they are not considered to be consuming them. I guess this was done for the sake of Android's pre-handler.

If we forget about backward compatibility, I'm certain this is the way to do it, but I don't see a good way to make this change while staying backward-compatible.

I find this to be a much clearer mental model.
* A handler can decide if it can process this particular exception.
* A handler can be in an invalid state, and it can now pass
  handling the uncaught exception to the system, without making
  additional noise.
* Before this change, there are *two* types of exception handlers:
  - Those that are used as coroutine context elements.
    If a coroutine context element processes an exception
    successfully, the system is not notified about them.
  - Those that are used as services.
    These are just notified about exception, they are not
    considered to be consuming them. I guess this was done for
    the sake of Android's pre-handler.

If we forget about backward compatibility, I'm certain this is the
way to do it, but I don't see a good way to make this change while
staying backward-compatible.
@dkhalanskyjb dkhalanskyjb marked this pull request as draft December 16, 2022 15:42
@elizarov
Copy link
Contributor

Btw, it looks like it can be made backwards compatible with JDK8 default interface methods.

@qwwdfsad
Copy link
Member

qwwdfsad commented Jan 5, 2023

If we forget about backward compatibility, I'm certain this is the way to do it

I tend to agree with this statement. Great job with figuring it out!

I would refine the API a bit to avoid situations where ordering is important by prohibiting user-defined platform exception handlers using any trick from our book (e.g. by introducing internal subinterface).

The good news are, @udalov is going to figure out jvm-defaults policy for Kotlin libraries migration soon, so we can definitely leverage it and migrate to tryHandleException

@dkhalanskyjb dkhalanskyjb force-pushed the TestScope-uncaught-exceptions-registry branch from 7695863 to b12a404 Compare February 9, 2023 14:49
@dkhalanskyjb dkhalanskyjb force-pushed the TestScope-uncaught-exceptions-registry branch 3 times, most recently from 2d5008a to 4fda39a Compare February 23, 2023 13:06
Base automatically changed from TestScope-uncaught-exceptions-registry to develop February 24, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants