Comments (11)
Thanks for the quick reply @mariusc83
I don't think this necessarily has to do with the use of a background thread, though the executor used here probably increases the delays.
Here's how I'd think about it:
- In a Java based runtime, a memory leak is a programming error that prevents the GC from collecting an object that is no longer needed.
- Activities and fragments should become unreachable immediately after their
onDestroy()
method is invoked. Preventing them from being garbage collected, even if it's only temporary, is still a memory leak. E.g. here if we were switching activities fast enough we'd be retaining quite a lot of memory. - This leak is particularly interesting in that the duration of the leak isn't well defined. It could be very short or very long, as it depends on a specific set of conditions: 1)
RumViewScope
staying as a child ofRumViewManagerScope
after the activity is destroyed 2) Any StopView event being sent close enough to a GC that the GC sees the java local reference when running and concludes that the object is reachable. Unfortunately, the more you use an app, the more the GC runs but also the more screens go in and out and the more we get StopView events.
I think long term I'd suggest something like: 1) Stop using the actual activity & fragment instances as keys, create unique identifiers for those instead (I don't know if you have any code expecting actual activity / fragment instances as keys though?). then 2) Stop using a weak ref, just keep a strong reference to the key, and make it clear that key should be an identifier not a live UI object.
from dd-sdk-android.
Hi @pyricau, thank you for all the details provided regarding this bug, tremendous work there.
This is indeed a very interesting discovery, so if I understand correctly because the onStopView
is being called in a different Thread
by using the keyRef.get()
in there we are impeding that activity
or fragment
to be recycled in the GC cycle that could run in parallel. Is that correct ? Please correct me if I am wrong but this doesn't mean that the activity or fragment will always be leaked, is just that this is not going to be GC fast enough ?
In any case we are going to fix this on our next version probably by just applying the quick fix first but on the long term I totally agree with you that we should find a better equivalent for those keys.
from dd-sdk-android.
Could we call WeakReference.refersTo instead? The docs say "Prefer this to a comparison with the result of get"
from dd-sdk-android.
@pyricau pointed out that refersTo
is only available on 33+. :( I'll close my PR.
If there's interest I can do something similar with System.identityHashCode. i.e. I could avoid calling WeakReference.get() except when the identity hash codes matched. This would be the case when they were the same object, but in such cases the object would definitely still be alive, so there would be no leak problem created. It would be very unlikely to happen spuriously since identity hash codes are 32 bits.
I'm happy to prepare a PR if you're interested in such a fix. Let me know.
from dd-sdk-android.
@tcmulcahy thank you for all the suggestions, we discussed it internally and we will take the time to prepare a more robust solution there. We will handle this on our end. As @pyricau was pointing out and I also agree with, the best approach would be to not have those activities/fragments as keys in there and totally remove the WeakRef
. As that code there is quite complex and has a lot of implications we will need more time to assess and properly fix this in a PR.
On the other hand in the meantime we will add the quick fix for our next sdk version by not accessing the reference if the view was already stopped.
Let me know if this is ok with you @pyricau @tcmulcahy.
from dd-sdk-android.
Sounds good! I had a PR with the quick fix at #1763 but happy to close it.
from dd-sdk-android.
from dd-sdk-android.
@pyricau no need to close it, the PR is actually good, I just approved it and we will merge it soon.
from dd-sdk-android.
The issue referenced is fixed in #1779, but we will keep this ticket open for the remaining work to be done.
from dd-sdk-android.
The fix for this issue is now available in version 2.4.0.
from dd-sdk-android.
Hi @pyricau! We've made changes to our code and no more hold Activities inside the RUM processing pipeline. This change was released in version 2.5.0.
I am closing this issue, don't hesitate to re-open it if needed.
from dd-sdk-android.
Related Issues (20)
- integration issue HOT 1
- Lower than expected startup times since 2.6.0 HOT 2
- Logging: setNetworkInfoEnabled(false) doesn't seem to take effect HOT 5
- ANR on `Rum.enable()` HOT 6
- Rum works but logger don't send logs HOT 6
- Remove a default attribute HOT 2
- Crash generated by DrawableUtils HOT 2
- JankStatsActivityLifecycleListener.onActivityStopped - HOT 4
- setTelemetrySampleRate, explanation HOT 3
- Security vulnerability introduced with SDK v2.7.1 (okio) HOT 2
- Unable to log custom actions HOT 1
- Memory Leak : AggregatingVitalMonitor listeners are never unregistered HOT 3
- Missing DataDog events caused by UnknownError HOT 9
- Cannot implementation com.datadoghq:dd-sdk-android-logs HOT 4
- ./gradlew uploadMappingRelease crashes trying to upload mapping.txt file after release build HOT 2
- How to group views as Service? HOT 1
- Fragment views stopped getting registered HOT 1
- Crash gets assigned to wrong view HOT 1
- Fatal Exception: java.lang.IllegalStateException Recording currently in progress - missing #endRecording() call? HOT 3
- What's the preferred `DataDogInterceptor` sampling rate? HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from dd-sdk-android.