Repository navigation
docs: Allow broad catches only at SDK boundaries #6187
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,30 +130,31 @@ The repository is organized into multiple modules: | |
|
|
||
| ### Exception Handling | ||
|
|
||
| **Never introduce a new `catch (Throwable)`.** Catch the narrowest type the guarded code can | ||
| actually throw. The repository still contains many pre-existing broad catches; they are legacy, | ||
| not a precedent to follow. | ||
|
|
||
| A broad catch swallows `OutOfMemoryError`, `StackOverflowError`, `ThreadDeath` and `LinkageError` — | ||
| conditions the JVM/ART cannot recover from and that leave the process in an undefined state — and | ||
| it hides real bugs in our own code behind a log line. | ||
|
|
||
| "The SDK must never crash the host application" is not a reason to catch `Throwable`. That goal is | ||
| served by `io.sentry.util.ExceptionUtils.rethrowIfFatal`, which lets the non-recoverable throwables | ||
| through while leaving everything else for the caller to log or ignore: | ||
|
|
||
| ```java | ||
| try { | ||
| doSomethingRisky(); | ||
| } catch (Throwable t) { | ||
| ExceptionUtils.rethrowIfFatal(t); | ||
| options.getLogger().log(SentryLevel.ERROR, "Failed to do something risky", t); | ||
| } | ||
| ``` | ||
|
|
||
| Apply that pattern only where a broad catch is genuinely unavoidable — an entry point that runs | ||
| arbitrary user code or third-party callbacks. Everywhere else, name the exception types. Say in the | ||
| PR description why the broad catch is necessary. | ||
| Inside the SDK, **never add a broad catch** (`Throwable`, `Error`, `Exception`, | ||
| `RuntimeException`). Catch the narrowest type the guarded code can actually throw. The many | ||
| existing broad catches are legacy, not precedent: they hide real bugs in our own code and make it | ||
| appear that Sentry is broken. | ||
|
|
||
| A broad catch is allowed only at a boundary where we can't know what the other side throws. Two | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't explicitly list user callbacks here since I don't think we have an agreement on what to do about those here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I thought the outcome from the last roundtable was that we must catch exceptions thrown by user callbacks 😢 ) |
||
| examples: | ||
|
|
||
| - public API entry points like `captureException` | ||
| - Android framework calls that cross into another process (`ContentResolver`, system services) | ||
|
|
||
| At a boundary: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general follow the spec in sentry-docs otherwise if nothing is specified use the following guidelines: |
||
|
|
||
| - Call `ExceptionUtils.rethrowIfFatal(t)` first so `OutOfMemoryError`, `StackOverflowError`, etc. | ||
| are never swallowed. It also rethrows `LinkageError`, so code probing an optional `compileOnly` | ||
| dependency must catch the specific subclass (`NoClassDefFoundError`, ...) before it. | ||
| - A code comment is required saying why the catch is broad and what happens on failure (event | ||
| dropped, feature disabled, ...). | ||
| - If wrapping user code with a broad catch, documentation is **required** explaining how the user | ||
| is expected to discover that the code they wrote isn't working. Both in code and in sentry-docs. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for public methods this should be on the javadoc |
||
| - Logging is not handling: `options.getLogger()` is silent unless `debug` is enabled, so a failure | ||
| that is only logged is invisible in production. | ||
|
|
||
| Instrumentation that wraps user code is not a boundary for the user's own exceptions: let them | ||
| propagate unchanged. | ||
|
|
||
| ### Testing Requirements | ||
| - Write comprehensive unit tests for new features | ||
|
|
||
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.
👍