ConditionalWeakTable causes a memory leak if one of their values references the table

[kpreisser]

Hi,

I found an odd behavior of System.Runtime.CompilerServices.ConditionalWeakTable<TKey, TValue> in both .NET Core and .NET Framework which looks like a bug to me: If you create multiple instances of the ConditionalWeakTable and store a key-value pairs in them, where the key stays alive and the value contains a reference to the ConditionalWeakTable, the values are not garbage-collected after they (and the ConditionalWeakTables) are no longer referenced.

For example, create a .NET Core Console application with the following code:

using System;
using System.Runtime.CompilerServices;

namespace ConsoleApp
{
    class Program
    {
        static void Main(string[] args)
        {
            object key = new object();
            while (true) {
                var table = new ConditionalWeakTable<object, Tuple<object, byte[]>>();
                table.Add(key, new Tuple<object, byte[]>(table, new byte[1000000]));

                GC.Collect();
            }
        }
    }
}

Expected behavior: The memory consumption of the program should stay in the same area, because when a new ConditionalWeakTable instance is created, there are no more references to the previous ConditionalWeakTable and its Tuple value, so they should be able to be reclaimed by the Garbage Collector.

Actual behavior: The memory consumption rises rapidly (4 GB after some seconds) until an OutOfMemoryException is thrown, as the byte arrays are not reclaimed by the garbage collector.

However, if you remove the reference to the table by replacing table.Add(...) with table.Add(key, new Tuple<object, byte[]>(null, new byte[1000000])), the problem disappears.

If the algorithm cannot be implemented such that it can detect that there are no more references to the table and its values, I think the ConditionalWeakTable should implement a Dispose() method that allows to clear all key-value-pairs.

The behavior is the same for .NET Core (.NETCoreAPP 1.1) and .NET Framework 4.6.2.

Thanks!

[stephentoub]

cc: @Maoni0

[Maoni0]

You are inducing gen2 GCs, so GC doesn't participate in deciding what's live and what's dead - it's purely determined by the user code. If you use the !gcroot sos command, it should shed some light as to what's holding an object alive.

[kpreisser]

Hi @Maoni0,
sorry, I'm not sure I understand you correctly. I included the GC.Collect() call just to show that even after an explicit GC the objects are not collected, but the behavior is the same when you remove this line.

You can reproduce the leak by running the above code as a .NET Core Console Application (VS target ".NETCoreApp 1.1") on Windows: After some seconds, the programm will crash.

Thanks!

[Maoni0]

In a blocking gen2 GC, GC does not participate in determining object's lifetime at all. So if a blocking gen2 does not collect something, it means that something is held alive by user code. The fact that you keep doing blocking gen2's and the object is not going away, it simply means GC is being told that it's alive. Does this make sense? I was trying to help you to find out who's holding it live. You can use !gcroot (I think gcroot does look at the dependent handles); !gchandles will show you what handles there are and what objects they hold onto.

[jkotas]

@maonis There is no user (ie non-framework) code keeping anything alive in the example above. The ConditionalWeakTable is keeping itself alive because of how it is implemented using conditional handles.

@kpreisser You should be able to workaround the issue by storing the back-references to ConditionalWeakTable in another weak reference, like table.Add(key, new Tuple<object, byte[]>(new WeakReference(table), new byte[1000000]));.

[Maoni0]

@jkotas by user code I just meant not GC code...

[kpreisser]

Hi @Maoni0,
Yes, it makes sense, thank you. But note: In my report I did not want to say that there's a problem with GC (in that it would not collect objects to which no more references exist) - I think GC is working correctly. Rather, it is the ConditionalWeakTable which somehow keeps references to the value objects alive even if no more non-framework code contains references to them and to the table, thus preventing GC from collecting the objects. This is what I wanted to show in the report.
I will try to see if I can run the commands which you suggested, thanks!

Hi @jkotas,
thank you. The above example code is only a simplified reproduction which I observed in a more complex application (although in .NET Framework, but the behavior is the same as on .NET Core on Windows), where the value objects need a reference to the table in order to get access to other attached objects.
Until now, I worked-around the issue by calling the internal ConditionalWeakTable.Clear() method using reflection, but with using your tip I was able to resolve the issue by clearing-out the references to the ConditionalWeakTable in the value objects after I no longer need the table, which is cleaner than using reflection. (Simply using a WeakReference to store the table would not have worked in my case, as I would need to ensure the table is not collected until I no longer need it, so I would have to store a strong reference to it on some other place).

Thanks!

[jkotas]

I worked-around the issue by calling the internal ConditionalWeakTable.Clear() method using reflection,

BTW: The Clear method was added as public API in .NET Core 2.0.

[Torvin]

@kpreisser I don't see this as a bug. I think it works exactly as expected. The whole purpose of ConditionalWeakTable is to tie the life time of the "right" part of the table to the life time of its "left" part. You effectively tied your table to your key, and since the object referenced by key is still alive - so is table.

The purpose of ConditionalWeakTable is to add an 'extension' field to an arbitrary object that would work the same way normal fields work. So in your case, if your key simply had a normal field referencing your table - you would observe the same 'leak'.

What you are doing - you are basically relying on the fact that when ConditionalWeakTable is collected - it will remove all the stored key-value associations and thus would allow the collection of the 'values'. While it's logical to assume that, I wouldn't rely on it. I don't think it was intended to be used in this way. It's not even documented.

[kpreisser]

Hi @Torvin,
thanks for your reply!

However, I'm not sure if I follow: If it is expected that the value (right part) is tied to the life time of the key (left part), then I would have expected that the leak would also occur even if the value doesn't reference the ConditionalWeakTable, because the value would still reference the byte array and the key is still reachable.

However, if you change the line in the above code:

table.Add(key, new Tuple<object, byte[]>(table, new byte[1000000]));

to

table.Add(key, new Tuple<object, byte[]>(null, new byte[1000000]));

(so that the value doesn't contain a reference to the table), then the leak doesn't occur, meaning that the values are garbage-collected even though the key is still alive.
But as this behavior only occurs in a special circumstance (when the value contains a reference to the ConditionalWeakTable in which the value is stored), it seems like a bug to me.

Additionally, from reading the documentation of ConditionalWeakTable, I don't read it to mean that values are attached to keys permanently and are therefore not GCed even if the ConditionalWeakTable is CGed. Rather, it should only look like values being attached to keys, while internally they are stored in a separate dictionary/table to which the keys don't have any relation/reference.

Note, that ECMAScript (JavaScript) defines a WeakMap that has a similar concept like the ConditionalWeakTable in .NET: It allows to store key-value-pairs, where an entry in the WeakMap doesn't prevent the key from being garbage-collected (even if the value has a reference to the key):

If an object that is being used as the key of a WeakMap key/value pair is only reachable by following a chain of references that start within that WeakMap, then that key/value pair is inaccessible and is automatically removed from the WeakMap.

However, if run the same test in ECMAScript implementations like SpiderMonkey (Mozilla Firefox) and V8 (Google Chrome, also used in Node.js), a leak doesn't occur (except for Chakra (Microsoft Edge) which funnily seems to have exactly the same behavior as in .NET where a leak only occurs if the value has a reference to the WeakMap):

let key = new Object();

while (true) {
    let map = new WeakMap();
    map.set(key, [map, new ArrayBuffer(1000000)]);
}

Now, because the WeakMap in ECMAScript has a similar concept like the ConditionalWeakTable in .NET, it is used e.g. by Jurassic, a JavaScript Engine for .NET, to implement the WeakMap. Unfortunately, this means when running the above JavaScript code in Jurassic, a memory leak will also occur here.

Thanks!

[Maoni0]

when you do this:

table.Add(key, new Tuple<object, byte[]>(table, new byte[1000000]));

a GC handle of the dependent type is created with its primary value as key and secondary value as the tuple obj. and this GC handle needs to be cleaned up by something - in the CWT case it's either by the finalizer or setting the primary of this handle to null (if you remove an entry). if you don't do either it'll stay and hold the secondary object live which means table will be alive. key can still be alive but if the handle is freed or if it's cleared as the primary for the GC handle it will not hold the secondary (tuple obj) alive.

[Torvin]

@kpreisser

However, if you change the line in the above code (so that the value doesn't contain a reference to the table), then the leak doesn't occur, meaning that the values are garbage-collected even though the key is still alive.

What I'm saying is - this behaviour (when collection of CWT also removes the GC handles) isn't documented and if I were you I wouldn't rely on it.

[kpreisser]

Hi @Torvin (sorry for the late reply),

OK, so this would mean that I need to clear the ConditionalWeakTable once I don't need it any more.
But this requires using reflection for .NET Framework (up to 4.7.2) since the .Clear() method is not public there (while for .NET Core it is). Also, as an API consumer, I would expect ConditionalWeakTable to implement IDisposable in such a case.

Otherwise, I don't think it is unreasonable to expect that GCing the table will also GC the handles since this is an implementation detail and the documentation doesn't indicate that special handling is required.

Thanks!

[pjanotti]

@jkotas @Maoni0 we could fix this in relatively simple way by creating a new struct to replace CWT usage of DependsHandle as the holder for key and value . This alternative struct could have a weak GCHandle to the key and a reference to the value. This way the change to the code will be very localized. At minimum some perf tests needs to be added to see how perf & mem will be affected, besides that, any concerns with this approach? Any gotchas that I should be aware?

[jkotas]

This alternative struct could have a weak GCHandle to the key and a reference to the value.

I do not think you would be able to maintain the CWT contract with this structure. It would be keeping the value alive for much longer (potentially infinitely long) once the entry becomes dead.