RUMS-4305 Fix the memory leak in the `PendingTrace#cleaner` by mariusc83 · Pull Request #2607 · DataDog/dd-sdk-android

mariusc83 · 2025-04-17T09:30:03Z

What does this PR do?

After analysing the borrowed code from the deprecated APM java tracer following some internal investigations PendingTrace was actually leaking in 2 places:

In one place when using dropSpan the cleaner could not actually clean the PendingTrace reference and this was persisted forever in the pendingTraces set. Basically when the cleaner was trying to clean that method was doing nothing as there was no pending Span to write and the reference was not removed.
Because of using a mutable LinkedList as an entry key to a Set<PendingTrace>. Basically because the key is resolved from the LinkedList#hashCode() method which is dependent on the values contained in the list first time a PendingTrace is created the hashCode() is always 1 and when later we're trying to remove it through clean the hashCodes no longer match.
The problem was not that visible when instrumenting the httpRequest as the hashCode when adding the list was always 1 so only 1 PendingTrace was persisted but it became more visible when using a very trivial example as:

val tracer = GlobalTracer.get()
            repeat(100){
                val span = tracer.buildSpan("AsyncOperation")
                    .start()
                Thread.sleep(1)
                span.finish()
            }

In this moment we're going to leak 200 PendingTraces because:

when adding each first PendingTrace the set will contain [1]->[(pendingTrace1, key1)]
when adding second PendingTrace the set will check if key 1 exist and then will equal the keys by values but because at the lists are equaling the elements in the equal method in this moment the first trace has 1 element and the one to be added has 0 so they do not match so the set will contain `[1]->[(pendingTrace1, key1), (pendingTrace2, key2)]
when later trying to remove each of these it will first try to match the hashCode but because they all have elements in them the hashCode will not longer match 1 key so the will all be leaked.
This problem is not reproducible when using the DatadogInterceptor because there we are using drop instead of finish in the Span and this will not add the span into the PendingTrace a.k.a LinkedList. In that situation the pendingTraces set will only contain one empty PendingTrace: [1]->[(PendingTrace, key)]. Every time a new PendingTrace will be created it will match through hashCode and equal to the one in the Set and it will not be added. In this situation only the first added PendingTrace will be leaked and this is because of the reason number 1 described above.

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)

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)

datadog-datadog-prod-us1 · 2025-04-22T13:24:20Z

+    }
+
+    @Test
+    fun `M cleanup random traces W add and close`(forge: Forge) {


⚪ Code Quality Violation

Function names should be camel case. (...read more)

This rule mandates that function names in Kotlin should be written in camel case, starting with a lowercase letter using no underscores. This is convention in Kotlin and enhances readability and maintainability.

datadog-datadog-prod-us1 · 2025-04-22T13:24:20Z

+    }
+
+    @Test
+    fun `M not leak the PendingTraces W dropSpan`(


⚪ Code Quality Violation

Function names should be camel case. (...read more)

This rule mandates that function names in Kotlin should be written in camel case, starting with a lowercase letter using no underscores. This is convention in Kotlin and enhances readability and maintainability.

datadog-datadog-prod-us1 · 2025-04-22T13:24:20Z

+@Extensions(
+    ExtendWith(MockitoExtension::class),
+    ExtendWith(ForgeExtension::class)
+)


⚪ Code Quality Violation

Annotations should be separated by a single line break. (...read more)

Each annotations should be placed on a separate line. This is important because it makes the code more readable and easier to understand. When multiple annotations are placed on the same line, it can become difficult to distinguish between them and understand their individual impacts on the associated code.

datadog-datadog-prod-us1 · 2025-04-22T13:24:20Z

+    }
+
+    @Test
+    fun `M not leak the PendingTraces W finishSpan from different threads`(


⚪ Code Quality Violation

Function names should be camel case. (...read more)

This rule mandates that function names in Kotlin should be written in camel case, starting with a lowercase letter using no underscores. This is convention in Kotlin and enhances readability and maintainability.

datadog-datadog-prod-us1 · 2025-04-22T13:24:20Z

+        assertThat(PendingTrace.getPendingTracesSize()).isEqualTo(0)
+    }
+
+    @Test


⚪ Code Quality Violation

Annotations should be separated by a single line break. (...read more)

Each annotations should be placed on a separate line. This is important because it makes the code more readable and easier to understand. When multiple annotations are placed on the same line, it can become difficult to distinguish between them and understand their individual impacts on the associated code.

datadog-datadog-prod-us1 · 2025-04-22T13:24:21Z

+    }
+
+    @Test
+    fun `M contain only one entry W add {same PendingTrace mutated, twice}`(forge: Forge) {


⚪ Code Quality Violation

Function names should be camel case. (...read more)

This rule mandates that function names in Kotlin should be written in camel case, starting with a lowercase letter using no underscores. This is convention in Kotlin and enhances readability and maintainability.

datadog-datadog-prod-us1 · 2025-04-22T13:24:21Z

+    // region concurrency
+
+    @Test
+    fun `M not leak any PendingTrace W add, mutate and remove { different threads }`(forge: Forge) {


⚪ Code Quality Violation

Function names should be camel case. (...read more)

This rule mandates that function names in Kotlin should be written in camel case, starting with a lowercase letter using no underscores. This is convention in Kotlin and enhances readability and maintainability.

datadog-datadog-prod-us1 · 2025-04-22T13:24:21Z

+                addCountDownLatch.countDown()
+            }.start()
+        }
+        pendingTraces.takeLast(pendingTraces.size / 2).forEach { pendingTrace ->


⚪ Code Quality Violation

Suggested change

pendingTraces.takeLast(pendingTraces.size / 2).forEach { pendingTrace ->

pendingTraces.takeLast(pendingTraces.size / 2).forEach {

pendingTrace ->

Block body statement on the same line as curly brace (...read more)

When using a multi-line statement with a curly brace, you should always put the contained code on a separate line from the curly brace.

The one exception to this is when using a lambda expression.

datadog-datadog-prod-us1 · 2025-04-22T13:24:21Z

+    override fun createEqualInstance(source: IdentityKey, forge: Forge): IdentityKey {
+        return IdentityKey(source.key)
+    }


⚪ Code Quality Violation

Function return type should be preceded by a single space. (...read more)

Kotlin enforces consistent spacing around the function return type: one space before the return type.

datadog-datadog-prod-us1 · 2025-04-22T13:24:21Z

+    // region `equals` contract
+
+    @Test
+    fun `M contain only one entry W add same PendingTrace twice`(forge: Forge) {


⚪ Code Quality Violation

Function names should be camel case. (...read more)

This rule mandates that function names in Kotlin should be written in camel case, starting with a lowercase letter using no underscores. This is convention in Kotlin and enhances readability and maintainability.

codecov-commenter · 2025-04-22T13:50:39Z

Codecov Report

Attention: Patch coverage is 68.82353% with 53 lines in your changes missing coverage. Please review.

Project coverage is 70.04%. Comparing base (c5b619f) to head (30caa79).
Report is 140 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ain/java/com/datadog/opentracing/PendingTrace.java	68.82%	34 Missing and 19 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2607      +/-   ##
===========================================
+ Coverage    69.97%   70.04%   +0.07%     
===========================================
  Files          820      820              
  Lines        30554    30568      +14     
  Branches      5130     5133       +3     
===========================================
+ Hits         21379    21410      +31     
+ Misses        7747     7723      -24     
- Partials      1428     1435       +7

Files with missing lines	Coverage Δ
...ain/java/com/datadog/opentracing/PendingTrace.java	`68.82% <68.82%> (+10.49%)`	⬆️

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

0xnm · 2025-04-23T08:39:01Z

+        pendingTraces.forEach {
+            testeCleaner.addPendingTrace(it)
+        }
+        assertThat(testeCleaner.pendingTraces.values).containsExactlyInAnyOrderElementsOf(pendingTraces)


should it be assume here as precondition?

Let me think...

I don't like assume as it doesn't have all the list assertions :(

yes, but it is a different behavior - if assume fails, then test will be simply skipped and it won't be counted as test failure, unlike in the assert case. Do we want to fail the test on this pre-condition check?

yup because that means the cleaner doesn't work correctly.

0xnm · 2025-04-23T08:47:17Z

+                if (forge.aBool()) {
+                    repeat(forge.anInt(min = 1, max = 10)) {
+                        pendingTrace.addSpan(mock())
+                    }
+                }
+                testeCleaner.addPendingTrace(pendingTrace)
+                repeat(forge.anInt(min = 1, max = 10)) {
+                    pendingTrace.addSpan(mock())
+                }
+                addCountDownLatch.countDown()


there is a small difference with a block above, it is using if (forge.aBool()), is it intended?

yeah I just wanted to test random case where the pending trace has something before was added, maybe should I add 2 tests here ? Isn't going to be too much ?

datadog-datadog-prod-us1 · 2025-04-23T09:14:06Z

+                addCountDownLatch.countDown()
+            }.start()
+        }
+        pendingTraces.subList(half, pendingTraces.size).forEach { pendingTrace ->


⚪ Code Quality Violation

Suggested change

pendingTraces.subList(half, pendingTraces.size).forEach { pendingTrace ->

pendingTraces.subList(half, pendingTraces.size).forEach {

pendingTrace ->

Block body statement on the same line as curly brace (...read more)

When using a multi-line statement with a curly brace, you should always put the contained code on a separate line from the curly brace.

The one exception to this is when using a lambda expression.

datadog-datadog-prod-us1 · 2025-04-23T09:14:06Z

+        assertThat(PendingTrace.getPendingTracesSize()).isZero()
+    }
+
+    @Test


⚪ Code Quality Violation

Annotations should be separated by a single line break. (...read more)

Each annotations should be placed on a separate line. This is important because it makes the code more readable and easier to understand. When multiple annotations are placed on the same line, it can become difficult to distinguish between them and understand their individual impacts on the associated code.

datadog-datadog-prod-us1 · 2025-04-23T09:14:06Z

+        val addCountDownLatch = CountDownLatch(pendingTraces.size)
+        val removeCountDownLatch = CountDownLatch(pendingTraces.size)
+        val half = pendingTraces.size / 2
+        pendingTraces.take(half).forEach { pendingTrace ->


⚪ Code Quality Violation

Suggested change

pendingTraces.take(half).forEach { pendingTrace ->

pendingTraces.take(half).forEach {

pendingTrace ->

Block body statement on the same line as curly brace (...read more)

When using a multi-line statement with a curly brace, you should always put the contained code on a separate line from the curly brace.

The one exception to this is when using a lambda expression.

datadog-datadog-prod-us1 · 2025-04-23T11:40:23Z

+        assertThat(PendingTrace.getPendingTracesSize()).isEqualTo(0)
+    }
+
+    @Test


⚪ Code Quality Violation

Annotations should be separated by a single line break. (...read more)

Each annotations should be placed on a separate line. This is important because it makes the code more readable and easier to understand. When multiple annotations are placed on the same line, it can become difficult to distinguish between them and understand their individual impacts on the associated code.

datadog-datadog-prod-us1 · 2025-04-23T11:40:24Z

+    }
+
+    @Test
+    fun `M contain 2 entries W add {2 PendingTrace instances with same root span}`(forge: Forge) {


⚪ Code Quality Violation

Function names should be camel case. (...read more)

This rule mandates that function names in Kotlin should be written in camel case, starting with a lowercase letter using no underscores. This is convention in Kotlin and enhances readability and maintainability.

ambushwork

LGTM

xgouchet

Nice work

0xnm · 2025-04-24T07:32:02Z

+        }
+        if (count > 0) {
+            // TODO attempt to flatten and report if top level spans are finished. (for accurate metrics)
+        } else if (pendingReferenceCount.get() == 0) {


I'm not quite sure about this change: let's say we created a new PendingTrace without any spans inside and we add spans only after first cycle of the SPAN_CLEANER is completed - with this change removePendingTrace will be called even before we manage to register first span, no? 🤔

hmm, you might be right, let me double check this. Idea is that I need to find a way to remove that trace somehow, without knowing all the code base it's hard :(.

0xnm · 2025-04-24T07:37:12Z

                        "childSpan_$i",
                        internalLogger
                    )
-                    pendingTrace.registerSpan(span)


what is the need for this change?

the registerSpan it is already called in the constructor of DdSpan

0xnm · 2025-04-24T07:40:09Z

+        Assumptions.assumeTrue(testedCleaner.pendingTraces.values.containsAll(pendingTraces))
+
+        // When
+        pendingTraces.shuffled().forEach {


Suggested change

pendingTraces.shuffled().forEach {

pendingTraces.shuffled(Random(forge.seed)).forEach {

but what is the need to call shuffled anyway? to have a different order between addPendingTrace and removePendingTrace calls?

mariusc83 self-assigned this Apr 17, 2025

mariusc83 force-pushed the mconstantin/rums-4305/fix-pendingtrace-memory-leak branch from 583cdda to d4ba379 Compare April 22, 2025 13:23

datadog-datadog-prod-us1 Bot reviewed Apr 22, 2025

View reviewed changes

mariusc83 marked this pull request as ready for review April 22, 2025 17:01

mariusc83 requested review from a team as code owners April 22, 2025 17:01

0xnm reviewed Apr 23, 2025

View reviewed changes

mariusc83 force-pushed the mconstantin/rums-4305/fix-pendingtrace-memory-leak branch from d4ba379 to 22d8940 Compare April 23, 2025 09:11

datadog-datadog-prod-us1 Bot reviewed Apr 23, 2025

View reviewed changes

ambushwork reviewed Apr 23, 2025

View reviewed changes

Comment thread features/dd-sdk-android-trace/src/test/kotlin/com/datadog/opentracing/SpanCleanerTest.kt Outdated

ambushwork reviewed Apr 23, 2025

View reviewed changes

Comment thread features/dd-sdk-android-trace/src/main/java/com/datadog/opentracing/PendingTrace.java

mariusc83 force-pushed the mconstantin/rums-4305/fix-pendingtrace-memory-leak branch 2 times, most recently from 7b49bdb to 604b091 Compare April 23, 2025 11:37

datadog-datadog-prod-us1 Bot reviewed Apr 23, 2025

View reviewed changes

ambushwork previously approved these changes Apr 23, 2025

View reviewed changes

mariusc83 requested a review from 0xnm April 23, 2025 14:03

mariusc83 dismissed ambushwork’s stale review via ef743a1 April 23, 2025 14:19

mariusc83 force-pushed the mconstantin/rums-4305/fix-pendingtrace-memory-leak branch from 604b091 to ef743a1 Compare April 23, 2025 14:19

xgouchet previously approved these changes Apr 23, 2025

View reviewed changes

mariusc83 requested a review from ambushwork April 23, 2025 15:37

0xnm reviewed Apr 24, 2025

View reviewed changes

mariusc83 dismissed xgouchet’s stale review via 9528106 April 24, 2025 10:45

mariusc83 force-pushed the mconstantin/rums-4305/fix-pendingtrace-memory-leak branch from ef743a1 to 9528106 Compare April 24, 2025 10:45

mariusc83 requested a review from 0xnm April 24, 2025 11:00

mariusc83 force-pushed the mconstantin/rums-4305/fix-pendingtrace-memory-leak branch from 9528106 to 1388482 Compare April 24, 2025 12:18

0xnm approved these changes Apr 24, 2025

View reviewed changes

RUM-9539 Fix the memory leaks in the PendingTrace

30caa79

mariusc83 force-pushed the mconstantin/rums-4305/fix-pendingtrace-memory-leak branch from 1388482 to 30caa79 Compare April 24, 2025 14:55

mariusc83 merged commit 2e24bf5 into develop Apr 24, 2025

mariusc83 deleted the mconstantin/rums-4305/fix-pendingtrace-memory-leak branch April 24, 2025 17:01

0xnm mentioned this pull request Jul 11, 2025

OutOfMemory crash due to lot of threads spawned named "dd-trace-monitor" #2785

Closed

	pendingTraces.takeLast(pendingTraces.size / 2).forEach { pendingTrace ->
	pendingTraces.takeLast(pendingTraces.size / 2).forEach {
	pendingTrace ->

	pendingTraces.subList(half, pendingTraces.size).forEach { pendingTrace ->
	pendingTraces.subList(half, pendingTraces.size).forEach {
	pendingTrace ->

	pendingTraces.take(half).forEach { pendingTrace ->
	pendingTraces.take(half).forEach {
	pendingTrace ->

	pendingTraces.shuffled().forEach {
	pendingTraces.shuffled(Random(forge.seed)).forEach {

Uh oh!

Conversation

mariusc83 commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

datadog-datadog-prod-us1 Bot Apr 22, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

datadog-datadog-prod-us1 Bot Apr 22, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

datadog-datadog-prod-us1 Bot Apr 22, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

datadog-datadog-prod-us1 Bot Apr 22, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

datadog-datadog-prod-us1 Bot Apr 22, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

datadog-datadog-prod-us1 Bot Apr 22, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

datadog-datadog-prod-us1 Bot Apr 22, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

datadog-datadog-prod-us1 Bot Apr 22, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

datadog-datadog-prod-us1 Bot Apr 22, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

datadog-datadog-prod-us1 Bot Apr 22, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

codecov-commenter commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

datadog-datadog-prod-us1 Bot Apr 23, 2025

Choose a reason for hiding this comment

⚪ Code Quality Violation

Uh oh!

datadog-datadog-prod-us1 Bot Apr 23, 2025

mariusc83 commented Apr 17, 2025 •

edited

Loading

codecov-commenter commented Apr 22, 2025 •

edited

Loading