Fix memory leak when tracing is enabled

[jamescrosswell]

Resolves #3431

Memory Profile prior to this PR

Memory Profile after this PR

[jamescrosswell]

In a follow up PR we might want to replace ConcurrentBag<T> with a hand crafted ConcurrentBagLite<T> or ConcurrentCollection<T>... as I don't think ConcurrentBag<T> is the right collection to be used here... see this stackoverflow thread. I think the bag is appropriate when you plan to take out all the items you put into it... so kind of like an any-in-any-out data structure.

We're using ConcurrentBag<T> in quite a few other places as well... and I suspect we don't need to - a much simpler data structure could serve our purposes.

[bitsandfoxes]

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?

[bitsandfoxes]

Do you know why this is required? Is something holding the reference to the TransactionTracer? Does that reference need to be released too?

[jamescrosswell]

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.