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

RUMM-2684: Observe uncaught exception in executors #1125

Conversation

0xnm
Copy link
Contributor

@0xnm 0xnm commented Nov 4, 2022

What does this PR do?

Turns out we were swallowing uncaught exceptions raised during the execution of the tasks submitted via Executor#execute, ExecutorService#submit or ScheduledExecutorService#schedule calls, because the behavior is like the following:

  • If uncaught exception is raised for task submitted via Executor#execute, then worker thread is killed and exception is printed to the console.
  • If uncaught exception is raised for task submitted via ExecutorService#submit or ScheduledExecutorService#schedule, then worker thread is kept running and exception is thrown on the thread is which doing Future#get call (normally the thread getting the result). We were always ignoring the Future returned after the task is submitted, so we were swallowing such exceptions.

This change closes this gap by adding the hook to the ThreadPoolExecutor#afterExecute method, which allows to observe such uncaught exceptions. ThreadPoolExecutor#afterExecute is called on the thread which did the actual task execution.

The logic of this hook is taken from the afterExecute documentation.

Exceptions will be sent to the devLogger and to the internal telemetry as well.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@0xnm 0xnm requested a review from a team as a code owner November 4, 2022 09:21
Base automatically changed from nogorodnikov/rumm-2700/use-eventwritecontext-for-logs to feature/sdkv2 November 4, 2022 09:47
Copy link
Collaborator

@mariusc83 mariusc83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2022

Codecov Report

Merging #1125 (b241f40) into feature/sdkv2 (991ace4) will decrease coverage by 0.08%.
The diff coverage is 82.86%.

@@                Coverage Diff                @@
##           feature/sdkv2    #1125      +/-   ##
=================================================
- Coverage          82.52%   82.44%   -0.08%     
=================================================
  Files                347      349       +2     
  Lines              11355    11383      +28     
  Branches            1912     1914       +2     
=================================================
+ Hits                9370     9384      +14     
- Misses              1412     1419       +7     
- Partials             573      580       +7     
Impacted Files Coverage Δ
.../datadog/android/core/internal/thread/ThreadExt.kt 73.33% <66.67%> (-10.00%) ⬇️
...n/com/datadog/android/core/internal/CoreFeature.kt 91.13% <100.00%> (+0.07%) ⬆️
...in/com/datadog/android/core/internal/SdkFeature.kt 88.18% <100.00%> (ø)
...ernal/thread/LoggingScheduledThreadPoolExecutor.kt 100.00% <100.00%> (ø)
.../core/internal/thread/LoggingThreadPoolExecutor.kt 100.00% <100.00%> (ø)
...lin/com/datadog/android/rum/internal/RumFeature.kt 93.00% <100.00%> (ø)
...id/v2/core/internal/storage/ConsentAwareStorage.kt 98.89% <100.00%> (ø)
...android/log/internal/domain/DatadogLogGenerator.kt 95.50% <0.00%> (-2.70%) ⬇️
...rsistence/file/batch/PlainBatchFileReaderWriter.kt 86.27% <0.00%> (-1.96%) ⬇️
...ain/java/com/datadog/opentracing/PendingTrace.java 58.62% <0.00%> (-1.72%) ⬇️
... and 8 more

@xgouchet xgouchet added the size-medium This PR is medium sized label Nov 7, 2022
@0xnm 0xnm merged commit e3791e5 into feature/sdkv2 Nov 7, 2022
@0xnm 0xnm deleted the nogorodnikov/rumm-2684/observe_uncaught_exceptions_in_executors branch November 7, 2022 13:43
@xgouchet xgouchet added this to the 1.16.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-medium This PR is medium sized
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants