Fix memory leak when tracing is enabled#3432
Conversation
|
In a follow up PR we might want to replace We're using |
bitsandfoxes
left a comment
bitsandfoxes
left a comment
There was a problem hiding this comment.
That seems like quite the gotcha! Thanks for fixing that!
I'm not sure I understand where that leak is coming from tho. The fix implies that the SDK doesn't get rid of the TransactionTracer?
| // Release tracked spans | ||
| ReleaseSpans(); |
There was a problem hiding this comment.
Do you know why this is required? Is something holding the reference to the TransactionTracer? Does that reference need to be released too?
There was a problem hiding this comment.
I'm not sure I fully understand yet, no. The stack overflow thread linked above seems to imply ConcurrentBag<T> stores stuff in a thread local and leaves it there, unless you remove it. It seems improbable that we wouldn't have picked up on something so obvious years ago if that was the case though. That would mean massive memory leaks all over our SDK (we use ConcurrentBag<T> in a number of places).
I'll merge this PR, create a release, and then do a bit more digging on potentially replacing ConcurrentBag<T> with a light weight custom type that isn't so mysterious. That will require some benchmarking and testing though, as it would be replacing something quite foundational.
2 participants
Resolves #3431
Memory Profile prior to this PR
Memory Profile after this PR