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

catch throwable to suppress errors in Java #1812

Merged
merged 4 commits into from
Nov 18, 2021
Merged

Conversation

bruno-garcia
Copy link
Member

@bruno-garcia bruno-garcia commented Nov 17, 2021

The goal is to change code locations we suppress error to avoid the SDK bubbling errors to customers.

Firs try: I changed every match of catch (Exception to catch (Throwable. Not matching Kotlin code though, the main focus was to help avoid the case we had when debugging a crash on Unity with the Android SDK so mainly sentry-java and sentry-android-core.

Adding additional logging isn't in the scope of this PR. When it was simple I just added a debug log, but if no logger available, I added a TODO and we can have a follow up PR.

@maciejwalkowiak does this make sense on the Spring and other backend packages or should I revert?

Partially addresses #1806 (not doing the Kotlin code)

@bruno-garcia bruno-garcia marked this pull request as ready for review November 18, 2021 00:14
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2021

Codecov Report

Merging #1812 (8311e42) into main (2d41eca) will not change coverage.
The diff coverage is 27.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1812   +/-   ##
=========================================
  Coverage     75.72%   75.72%           
  Complexity     2194     2194           
=========================================
  Files           218      218           
  Lines          7806     7806           
  Branches        828      828           
=========================================
  Hits           5911     5911           
  Misses         1493     1493           
  Partials        402      402           
Impacted Files Coverage Δ
...ry/transport/apache/ApacheHttpClientTransport.java 68.91% <0.00%> (ø)
...main/java/io/sentry/spring/SentrySpringFilter.java 71.15% <0.00%> (ø)
...racing/SentrySpanClientHttpRequestInterceptor.java 0.00% <0.00%> (ø)
...ry/src/main/java/io/sentry/DirectoryProcessor.java 68.62% <0.00%> (ø)
...ry/SendCachedEnvelopeFireAndForgetIntegration.java 30.95% <0.00%> (ø)
...c/main/java/io/sentry/SentryCrashLastRunState.java 94.44% <0.00%> (ø)
.../main/java/io/sentry/SentryEnvelopeItemHeader.java 92.59% <0.00%> (ø)
...io/sentry/UncaughtExceptionHandlerIntegration.java 76.00% <0.00%> (ø)
...o/sentry/adapters/ContextsDeserializerAdapter.java 40.81% <0.00%> (ø)
...java/io/sentry/adapters/DateSerializerAdapter.java 42.85% <0.00%> (ø)
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d41eca...8311e42. Read the comment docs.

@maciejwalkowiak
Copy link
Contributor

I don't see how this could hurt anything server side related. Looks good to me.

@marandaneto
Copy link
Contributor

marandaneto commented Nov 18, 2021

Missing specific catch blocks that don't use Exception but rather something specific eg RuntimeException, InterruptedException, ParseException, and many more.

Missing changes on Kotlin projects.

the PR description states that logging isn't the focus but resolves the issue #1806
The PR description describes that logging isn't part of this PR so either we don't close (resolve) #1806 or we open a new issue, we don't want to silently swallow a Throwable and not log out, it'd be even harder than crashing for debugging eventual issues.

Tests are not adjusted to using a Throwable error instead of Exception, so we cannot guarantee that those changes don't have a side effect during code execution (at least when testing)

@bruno-garcia
Copy link
Member Author

Missing specific catch blocks that don't use Exception but rather something specific eg RuntimeException, InterruptedException, ParseException, and many more.

I wouldn't go that far. When we picked a specific exception type, the intent was to handle only that one. That implies already that any other type was supposed to bubble up so for that reason I wouldn't change those.

Missing changes on Kotlin projects.
the PR description states that logging isn't the focus but resolves the issue #1806 The PR description describes that logging isn't part of this PR so either we don't close (resolve) #1806 or we open a new issue, we don't want to silently swallow a Throwable and not log out, it'd be even harder than crashing for debugging eventual issues.

Fair point. I'll remove the Resolves from the PR. We can keep that for adding any missing logging and a linter so we don't have catch (Exception) anymore in the code base.

Tests are not adjusted to using a Throwable error instead of Exception, so we cannot guarantee that those changes don't have a side effect during code execution (at least when testing)

I'm not sure what we can test in this case. The intent of the code I changed was to suppress any unhandled error. It was done on the "wrong" exception type. I wrote many of those and as a C# dev I just wrote what I knew and missed the point Throwables could go through my catch block so it's just a correction of those.

@bruno-garcia bruno-garcia changed the title catch throwable to suppress errors catch throwable to suppress errors in Java Nov 18, 2021
@bruno-garcia
Copy link
Member Author

Discussed on a call with @marandaneto and agreed to merge this not as resolving the inital issue but partially addressing it.

@bruno-garcia bruno-garcia merged commit 380ea81 into main Nov 18, 2021
@bruno-garcia bruno-garcia deleted the ref/catch-throwable branch November 18, 2021 19:22
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.

4 participants