ConcurrentBag containing a circular reference does not get garbage collected

[halter73]

ConcurrentBag has a ThreadLocal field that maintains references to whatever was added to it without being removed/cleared even after the ConcurrentBag itself becomes unreferenced.

Use your favorite search engine to find references to ConcurrentBag leaking memory to see that this has been a gotcha for developers for a while now. I don't think it should be necessary to explicitly clear the ConcurrentBag when you're finished with it.

@vancem @stephentoub

[benaadams]

Does it need a finalizer? :-/

[stephentoub]

ThreadLocal has a finalizer.

Are you suggesting there's a leak even after the finalizer runs? If so, that should be fixed (can you share a repro?). If not, what exactly are you hoping to see here if you consider the finalizer insufficient and you don't want the developer to have to take an explicit deterministic action?

[halter73]

Interesting. I didn't include many details because I didn't realize this behavior, while unintuitive, was actually unexpected.

Could this issue possibly arise when the ConcurrentBag entry has a circular reference? I'm wondering if that's what's causing the ThreadLocal's finalizer not to run.

I noticed the leaked references when debugging a (most likely) unrelated issue in Kestrel's functional tests on Ubuntu 14.04.5 LTS with 2.0.0 prerelease corefx packages. Here's the offending ConcurrentBag in our test project FWIW.

I haven't tried simplifying the repro yet, but I can do so if that will help. Here's an example gcroot of the sort that started getting me suspicious of ConcurrentBag.

(lldb) dumpheap -mt 00007fb0b70bbeb0
         Address               MT     Size
...
00007faf643830f0 00007fb0b70bbeb0       40

Statistics:
              MT    Count    TotalSize Class Name
00007fb0b70bbeb0      339        13560 System.Collections.Concurrent.ConcurrentBag`1[[System.Object, System.Private.CoreLib]]
Total 339 objects

(lldb) gcroot 00007faf643830f0
Failed to enumerate GC references.
Failed to walk thread fde3
HandleTable:
    00007FB12F822AB0 (strong handle)
    -> 00007FAD644B9760 System.Object[]
    -> 00007FAC64290E70 System.Threading.ThreadLocal`1+LinkedSlotVolatile[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]][]
    -> 00007FAD64524BF0 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAF6426E8F8 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAE65082BF0 System.Threading.ThreadLocal`1+LinkedSlotVolatile[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]][]
    -> 00007FAC6428C220 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAF642F26D0 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAD64F5BA78 System.Threading.ThreadLocal`1+LinkedSlotVolatile[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]][]
    -> 00007FAC6425CEC0 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAC6424EE58 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAF642781D0 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAF642774B0 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAD64549048 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAD64528978 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAD64F3D848 System.Threading.ThreadLocal`1+LinkedSlotVolatile[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]][]
    -> 00007FAD645282A8 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAF6426F018 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAE644DCE58 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAC6420A6D8 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAF6425E838 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAE643DF708 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAC643ACA90 System.Threading.ThreadLocal`1+LinkedSlotVolatile[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]][]
    -> 00007FAE650C6B00 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAD64A91FE8 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAF64356BD8 System.Threading.ThreadLocal`1+LinkedSlotVolatile[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]][]
    -> 00007FAF64355CD8 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAC6439E128 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAC643A3D00 System.Threading.ThreadLocal`1+LinkedSlotVolatile[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]][]
    -> 00007FAD6520E0E0 System.Threading.ThreadLocal`1+LinkedSlot[[System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]], System.Collections.Concurrent]]
    -> 00007FAD6520E110 System.Collections.Concurrent.ConcurrentBag`1+WorkStealingQueue[[System.Object, System.Private.CoreLib]]
    -> 00007FAD6520E150 System.Object[]
    -> 00007FAD6520E0B8 Microsoft.AspNetCore.Hosting.Internal.HostingLoggerExtensions+HostingLogScope
    -> 00007FAD6520DFC0 Microsoft.AspNetCore.Http.DefaultHttpContext
    -> 00007FAD6520E038 Microsoft.AspNetCore.Http.Internal.DefaultHttpRequest
    -> 00007FAF64384450 Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Frame`1[[Microsoft.AspNetCore.Hosting.Internal.HostingApplication+Context, Microsoft.AspNetCore.Hosting]]
    -> 00007FAF64384410 Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.FrameContext
    -> 00007FAF64382FF8 Microsoft.AspNetCore.Testing.TestServiceContext
    -> 00007FAF64383170 Microsoft.AspNetCore.Testing.TestKestrelTrace
    -> 00007FAF64383048 Microsoft.AspNetCore.Testing.TestApplicationErrorLogger
    -> 00007FAF643830F0 System.Collections.Concurrent.ConcurrentBag`1[[System.Object, System.Private.CoreLib]]

Found 1 unique roots (run '!GCRoot -all' to see all roots).
(lldb)

[stephentoub]

Could this issue possibly arise when the ConcurrentBag entry has a circular reference?

Yes, if the bag contains a strong reference to itself, either directly or indirectly, that would prevent it from getting finalized until the thread containing the entry dies.

[vancem]

Hmm, this is unfortunate. Let me see if I understand.

Basically we have a ThreadLocal object, which controls a 'slot' in an array in each thread that lives as long as the thread. The ConcurrentBad has a ThreadLocal to maintain a set of per-thread 'WorkStealingQueue's which actually holds the values of the bag (basically each thread 'owns' a chunk of the bag). Thus the BAG points as the ThreadLocal, which just has the index into the per-thread array. Thus from the GC's point of view the BAG does not keep the items alive, it is the Thread (and its array) that does so. However when the BAG dies, it will kill the ThreadLocal which will remove the slot from the array and everything works.

However if the bag contains a reference to itself in one of its elements, then the Thread keeps the workstealingQueues alive which keep the user elements alive which keeps the bag alive which keep the ThreadLocal alive, so the ThreadLocal's finalizer never gets called, and we have the leak.

This is unfortunate, because if the bag contains 'big, global things' there is a reasonably good chance that there may be a reference that loops back to point at the bag, causing a leak. This is what his happening above. I suspect the bag holds the logging scope, which is just 'stuff' that the logging system might want, which holds the HTTPContext (very useful stuff for the logger), which holds a reference to the logging system which ultimately points back to the Scope (which is the Bag).

SOLUTION 1: DO NOTHING:
One solution is to say 'don't do that', that is make it the user's fault if you put an element into the bag that transitively refers to itself. This is a pretty bad solution because this bad behavior is a result of implementation decisions of ConcurrentBag (you an make a perfectly fine implementation of ConcurrentBag that does not have this problem). Moreover the scenarios where this causes grief are not that unusual (you just need 'big objects' that tend to transitively point at 'everything').

However this solution MAY be useful in the short term. If HostLoggingScope had a weak reference to its elements, then it would fix the problem in this scenario. Also I suspect a scope has an obvious 'end' and if so you could 'clear it' there, and thus break any loops. This may be the easiest short term solution.

SOLUTION2: (CONDITIONAL) Weak handles.

This problem of loops through 'handles' has occurred before. Indeed it was the reason for creating ConditionalWeakTable, and the ConditionalHandle that is used to implement it. If the per-thread array used by ThreadLocal contained a conditional weak handle that pointed at the T, and was conditional on the ThreadLocal being alive, then that would fix things. Bascially the GC would do the collection assuming the target was a weak reference, and if the ThreadLocal object was dead under those conditions, then the ThreadLocal would be collected. However if it was alive, then the conditional handle to the target would become strong, and everything that handle pointed at would also be kept alive.

I am worried that this may affect perf, since all accesses now have a few more indirections in the scheme above. Thus it has poor pay-for-play characteristics. I suppose we could make a 'fast' option on ThreadLocal which requires the user to avoid loops (that is what we have today), and that would be more pay-for-play.

Finally I note that this problem is really more an issue of ThreadLocal not of ConcurrentBag<K, T> Any code that uses ThreadLocal (or things like it), might have a similar issue.

We probably need to think about it a bit more, but I do think we should do something...

[Clockwork-Muse]

Um, out of curiosity, how long is a logging scope supposed to last?

[halter73]

@Clockwork-Muse It's only supposed to last the duration of request. In the Kestrel tests, we were storing the scope in TestApplicationErrorLogger and never clearing it, so in that case I expected it to be rooted for the duration of the test.