Introduce FailFast utils to provide offensive programing #5373
Conversation
|
@jpelgrom @dshokouhi if we agree with the implementation I'm going to write a small documentation in the developer documentation explaining a bit more how and when to use it. |
There was a problem hiding this comment.
I would first test with a condition to only crash while in debug, just to have an amostrage of how often this may happen in the current app state, afterwards I totally support the approach you are proposing.
Have in mind that crashes are something we need to fix quicker because depending where they are it can be a total blocker for the user, considering Google's "speed" recently on review approval, I would take that into consideration.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Today it will only crash in DEBUG meaning only when a developer would provide an APK manually. In RELEASE the default is to catch the error and log it and continue. It could put the app in an undefined state but at least it would not crash there. As it is state in the description I would like to have an opt-in feature in the app where ppl could agree to crash like in DEBUG. This could be useful for developers or user to give us more details. This PR is not covering this proposal. |
Regular users don't use logcat. I worry that this is too much formatting (trying to create a box) which gets in the way or is lost when reading/copying from the app's logs section. |
There was a problem hiding this comment.
if we agree with the implementation I'm going to write a small documentation in the developer documentation explaining a bit more how and when to use it.
Generally agreed this could be useful to have. Review comments are mostly small tweaks which shouldn't result in major changes that impact any documentation.
While developing sometimes we encounter places where it should never throw an exception but to add more safety this PR introduce a mechanism that will crash crash in DEBUG if this kind of issue are encounter.
Fail fast is not the only possible fix for that. "it should never throw" could also mean "it should always be caught (we expect this may produce exceptions)".
| this.handler = handler | ||
| } | ||
|
|
||
| fun failWhen(condition: Boolean, message: () -> String) { |
There was a problem hiding this comment.
Should this function and both failOnCatch functions have a short description as they are the 'public API'? (One or two sentences, not a full paragraph!)
| The exception is ignored to avoid a crash but it should be handled | ||
| if you see this please open an issue on github https://github.com/home-assistant/android/issues/new/choose |
There was a problem hiding this comment.
Using similar text to core requesting filing a bug report, full sentences, shorter link.
| The exception is ignored to avoid a crash but it should be handled | |
| if you see this please open an issue on github https://github.com/home-assistant/android/issues/new/choose | |
| The error has been ignored to avoid a crash, but it should be handled. | |
| Please create a bug report at https://github.com/home-assistant/android/issues/new. |
| additionalMessage?.let { | ||
| appendLine() | ||
| appendLine(it) | ||
| appendLine(SEPARATOR) |
There was a problem hiding this comment.
Why add another line at the end only if there is an additional message?
|
|
||
| class FailFastTest { | ||
|
|
||
| @Test |
There was a problem hiding this comment.
May I suggest another test for the fallback value?
| authenticationRepository(it.id).getSessionState() == SessionState.CONNECTED | ||
| FailFast.failOnCatch( | ||
| message = { | ||
| """Fail to get authenticationRepository for ${it.id} current authenticationRepos ids: ${authenticationRepos.keys} """ |
There was a problem hiding this comment.
| """Fail to get authenticationRepository for ${it.id} current authenticationRepos ids: ${authenticationRepos.keys} """ | |
| """Failed to get authenticationRepository for ${it.id}. Current repository ids: ${authenticationRepos.keys}.""" |
| FailFast.failOnCatch { | ||
| // Sometimes the service cannot be started as foreground due to the app being in a state where | ||
| // this is not allowed. We didn't identified yet how to avoid starting the service in this state. | ||
| // To avoid a crash, we catch it with FailFast, which will only crash in debug builds. |
There was a problem hiding this comment.
Isn't this line obvious, so unnecessary?
| LAUNCHER.onServiceCreated(this, notificationId, notification, type) | ||
| FailFast.failOnCatch { | ||
| // Sometimes the service cannot be started as foreground due to the app being in a state where | ||
| // this is not allowed. We didn't identified yet how to avoid starting the service in this state. |
There was a problem hiding this comment.
| // this is not allowed. We didn't identified yet how to avoid starting the service in this state. | |
| // this is not allowed. We haven't identified how to avoid starting the service in this state yet. |
jpelgrom
changed the title
Summary
While developing sometimes we encounter places where it should never throw an exception but to add more safety this PR introduce a mechanism that will crash crash in DEBUG if this kind of issue are encounter. It makes the error important to investigate ASAP.
It is also useful for unlocking an issue that happen in prod but we don't know how to reproduce it.
The design is flexible so that if we want we can override it so that the errors are reported to Sentry for instance. We could also add an option for us to enable the crash on release too. It would be useful for developer to get the error when we are in production.
Checklist
Screenshots
In debug the application process is killed and you have this message in the logs
In release the application is kept alive and you have this message in the logs
Link to pull request in documentation repositories
User Documentation: home-assistant/companion.home-assistant#
Developer Documentation: home-assistant/developers.home-assistant#
Any other notes
I've catch two errors that we have on Sentry that we don't know yet how to reproduce.