Make sure RumAppStartupDetector.destroy is called on main thread#3397
Conversation
0b8d0d4 to
14ad681
Compare
14ad681 to
856a87b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3397 +/- ##
===========================================
+ Coverage 72.06% 72.08% +0.02%
===========================================
Files 957 958 +1
Lines 35333 35340 +7
Branches 5863 5865 +2
===========================================
+ Hits 25461 25474 +13
- Misses 8261 8265 +4
+ Partials 1611 1601 -10
🚀 New features to boost your workflow:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 856a87bc7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| handler.post { | ||
| @Suppress("ThreadSafety") // handler posts to the main looper | ||
| detector?.destroy() | ||
| } |
There was a problem hiding this comment.
When onStop() is called from a worker thread, this posts destroy() and then immediately returns after nulling the field. SdkFeature.stop() treats wrappedFeature.onStop() as the teardown barrier and immediately clears the feature storage/context afterward, while RumAppStartupDetectorImpl.destroy() is the code that unregisters the application lifecycle callback and removes the first-frame view/window listeners. If Datadog.stopInstance() runs off-main while the main queue is busy, those callbacks remain active after the SDK has been stopped and can still run against a torn-down feature until this runnable is eventually processed; dispatching to main should wait for completion before onStop() returns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't know... Seems like an overkill tbh.
|
|
||
| rumAppStartupDetector?.destroy() | ||
| val detector = rumAppStartupDetector | ||
| if (Looper.myLooper() == Looper.getMainLooper()) { |
There was a problem hiding this comment.
Maybe worth to move this logic into some extension method for further use?
There was a problem hiding this comment.
done
added isMainThread function in dd-sdk-android-internal
ca280e9 to
dadf7b9
Compare
|
|
||
| Mockito.mockStatic(Looper::class.java).use { mockedLooper -> | ||
| // When | ||
| mockedLooper.`when`<Looper> { Looper.getMainLooper() }.thenReturn(mockMainLooper) |
There was a problem hiding this comment.
nit: you can use whenever to avoid adding `
4 participants
What does this PR do?
Addressing this comment #3373 (comment)
Observations:
CalledFromWrongThreadExceptionwhen callingremoveOnDrawListenerandremoveOnAttachStateChangeListeneron a background thread in a UI test, it doesn't crash. Nevertheless, looking at ViewRootImpl code, it seems it requires main thread for a lot of its methods. So, I would say we should be extra careful when interacting with "view related things" from background threads.RumAppStartupDetectorImplby itself doesn't support callingdestroyfrom non-main threads. All fields aren't volatile, there is no synchronization.Current PR:
UiThreadannotation toRumAppStartupDetector.destroyto establish explicit contract for this class.RumFeature.onStopmake sureRumAppStartupDetector.destroyis called from the main thread.Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)