Fix RumWindowCallbacksRegistry for multiple SDK instances#3003
Conversation
|
🎯 Code Coverage 🔗 Commit SHA: c3f7f8b | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3003 +/- ##
===========================================
+ Coverage 71.08% 71.26% +0.18%
===========================================
Files 859 859
Lines 31315 31320 +5
Branches 5276 5276
===========================================
+ Hits 22260 22320 +60
+ Misses 7552 7515 -37
+ Partials 1503 1485 -18
🚀 New features to boost your workflow:
|
|
|
||
| private fun Window.tryToRemoveCallback() { | ||
| val currentCallback = callback | ||
| if (currentCallback is RumWindowCallback) { |
There was a problem hiding this comment.
The problem is in this if (currentCallback is RumWindowCallback) check. Suppose we have 2 SDK instance and both installed their RumWindowCallbacks, wrapping the existing ones.
When removing the callback we need to check that we are removing "our" RumWindowCallback, but not the RumWindowCallback of the RumWindowCallbacksRegistry from a different SDK instance.
| callback = currentCallback.wrapped | ||
| private fun Window.tryToRemoveCallback() { | ||
| val currentCallback = callback | ||
| if (currentCallback is RumWindowCallback && currentCallback.registry === this@RumWindowCallbacksRegistryImpl) { |
There was a problem hiding this comment.
Probably alternative here is to check if RumWindowCallbacksRegistryImpl.callbacks.values() contains this callback, but we avoid doing this, because RumWindowCallbacksRegistryImpl.callbacks is a WeakHashMap?
I'm okay with solution if it don't impose leaks due to the circular dependency.
There was a problem hiding this comment.
Probably alternative here is to check if RumWindowCallbacksRegistryImpl.callbacks.values() contains this callback, but we avoid doing this, because RumWindowCallbacksRegistryImpl.callbacks is a WeakHashMap?
This solution will also work, I just didn't think about it. It is actually a bit cleaner I think. Fixed.
I'm okay with solution if it don't impose leaks due to the circular dependency.
Kotlin GC can deal with circular references. Unless this "circle" goes through some C++ code. But this isn't our case I think. Or are you talking about something else?
There was a problem hiding this comment.
It was just a raw assumption without going into the details.
Actually, we didn't check our SDK for leaks for a while, maybe we need to do a run of sample app with LeakCanary.
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
devflow unqueued this merge request: It did not become mergeable within the expected time |
dd-devflow
Bot
added
mergequeue-status: waiting
mergequeue-status: removed
and removed
mergequeue-status: waiting
labels
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
[email protected] cancelled this merge request build |
|
/merge -c |
|
View all feedbacks in Devflow UI.
|
dd-devflow
Bot
added
mergequeue-status: retrying
and removed
mergequeue-status: in_progress
mergequeue-status: retrying
labels
3 participants
What does this PR do?
Fixing
RumWindowCallbacksRegistryfor the case when there are multiple Datadog SDK instances in the app.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)