Repository navigation
ref: Narrow Throwable catches to the exceptions they guard (JAVA-552) - #6564
Draft
runningcode wants to merge 3 commits into
Draft
runningcode wants to merge 3 commits into
runningcode wants to merge 3 commits into
Conversation
Replace catch (Throwable) with the types the guarded code can throw, so SDK bugs and fatal errors are no longer swallowed: RejectedExecutionException around executor submit and schedule, IOException and SecurityException around file I/O, and the specific parse exceptions elsewhere. Drop the try where nothing in it can throw. Several tests only passed because these catches hid broken mocks: executor mocks that returned Unit from submit, mock Contexts without assets, a Runtime.exec that returned null. Make those mocks behave like the real objects. Refs JAVA-552 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
📲 Install BuildsAndroid
|
4 of 9 tasks
Their removal is in no/java-552-remove-unneeded-throwable-catches, so this branch only narrows. Refs JAVA-552 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 of 9 tasks
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📜 Description
Replace 80
catch (Throwable)with the exceptions the guarded code can throw. SDK bugs and fatalerrors at these sites are no longer swallowed:
RejectedExecutionExceptionaroundsubmitandschedule, whichISentryExecutorServicedeclares.SendCachedEnvelopeIntegrationalready caught it. Its broad arm is replaced by handlingInterruptedException(restoring the interrupt flag) andExecutionExceptionfromfuture.get(), next to the existingTimeoutException.IOException.SecurityExceptionforFile.delete,renameToandexists, which can throw nothing else.NumberFormatException,IllegalArgumentException,MalformedURLException,URISyntaxException,PatternSyntaxException, and so on.IndexOutOfBoundsException, which an incoming entrywithout
=triggers.The 15 catches that guard nothing are removed in #6565 instead.
Test changes
Several tests only passed because these catches hid mocks that don't behave like the real
objects. They now do:
Unitfromsubmitinstead of aFuture.ContextUtilsTestHelper.createMockContextgave the mockedContextandApplicationnoassets. A real
Contextalways has them.RootCheckerTest's mockedRuntime.execreturnednull.EnvelopeCacheTestfaked a session write failure with aRuntimeException; it now throws anIOException.submitthrowRejectedExecutionExceptioninstead of a plainRuntimeException.💡 Motivation and Context
This is part of the
catch (Throwable)audit:LinkageErroris expected.The three PRs don't depend on each other.
Baggage.javaconflicts with #6565, which re-indentsthe per-entry catch narrowed here, so whichever PR merges second needs a rebase.
Six sites the audit had marked for narrowing are left for the boundary PR:
DeviceInfoUtil.getExternalStorageStatcallscontext.getExternalFilesDirs, a framework call.writeToruns the app's own upload code.ViewHierarchyEventProcessoris part of the walk over the app's views.BuildInfoProvider.isEmulatorandContextUtils.getFamilyreadBuild.*fields. Making themnull-safe changes their signatures.
💚 How did you test it?
Ran the unit tests of
sentry,sentry-android-core,sentry-android-ndk,sentry-android-replay,sentry-async-profiler,sentry-apollo-3,sentry-apollo-4,sentry-jdbc,sentry-opentelemetry-coreandsentry-opentelemetry-otlp, plus detekt for theKotlin modules.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
rethrowIfFataland a comment saying why the catch is broad to the boundary sites, one PRper module group.
🤖 Generated with Claude Code