1

It seems the following code to unit test WeakReference does not work correctly/reliably:

object realObject = new object();
WeakReference weakReference = new WeakReference(realObject);
Assert.NotNull(weakReference.Target);
realObject = null;
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
Assert.Null(weakReference.Target);

The testing is run targeting both net461 and net5.0, for both DEBUG and RELEASE mode. The best result:

So the problem narrows down to targeting net5.0 DEBUG mode.

I also came across the following posts but it seems they're not very helpful mostly because they're out-of-date:

Worth to mention the answer from @Eric Lippert for Why C# Garbage Collection behavior differs for Release and Debug executables? says: You absolutely cannot rely on the garbage collector having any particular behaviour whatsoever with respect to the lifetime of a local, which suggests testing a WeakReference cannot be done reliably.

Edit: More information about the context of why using a WeakReference:

I'm developing a framework providing class Entity<T> and Record<T> similar to System.Data.DataRow and System.Data.DataRowView, with the most significance that the prior one is strongly typed. Entity is the model provides its own event to notify changes, Record<T> is a viewmodel that wraps Entity<T> to translate the change event into INotifyPropertyChanged plus other things such as databinding, validations, etc. Entity<T> should not aware the existence of Record<T>. To avoid memory leak, Record<T> should be GC reclaimed when only Entity is still referenced.

The example code for the event hookup:

IEntityListener Implemented by Record<T> class:

internal interface IEntityListener
{
    void OnFieldUpdated(FieldUpdatedEventArgs e);
}

WeakEntityListner to do the event hookup:

internal sealed class WeakEntityListener
{
    private readonly WeakReference<IEntityListener> _weakListener;
    private readonly Entity _entity;

    internal WeakEntityListener(Entity entity, IEntityListener listener)
    {
        _entity = entity;
        _weakListener = new (listener);
        _entity.FieldUpdated += OnFieldUpdated;
    }

    private void OnFieldUpdated(object? sender, FieldUpdatedEventArgs e)
    {
        if (_weakListener.TryGetTarget(out var listener))
            listener.OnFieldUpdated(e);
        else
            CleanUp();
    }

    private void CleanUp()
    {
        _entity.FieldUpdated -= OnFieldUpdated;
    }
}

I would like to have method OnFieldUpdated and CleanUp fully covered by unit test.

Weifen Luo
  • 603
  • 5
  • 13
  • IMHO It looks like you have already answered your own question. But *why* are you testing the behaviour of weak reference? What behaviour does you application depend on? – Jeremy Lakeman Nov 22 '21 at 02:08
  • The actual scenario is a custom weak event implementation using weak reference. I need to test the event handler will be unwired when the listener is garbage collected. The question is not fully answered because the testing code does not work targeting net5.0 in DEBUG mode. – Weifen Luo Nov 22 '21 at 03:02
  • 2
    Making the test case completely deterministic might be impossible. Even https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/tests/System/WeakReferenceTests.cs seems to only run if `PlatformDetection.IsPreciseGcSupported`. I would step back and ask why you are using weak references, and can you implement your requirements without them. – Jeremy Lakeman Nov 22 '21 at 03:09
  • WeakReference is the only practical option I know to implement a weak event: https://www.codeproject.com/Articles/29922/Weak-Events-in-C. Basically in the CLR world, WeakReference is the only option to allow referencing an object while still allowing that object to be reclaimed by garbage collection. Please correct me if I'm wrong. – Weifen Luo Nov 22 '21 at 03:19
  • The runtime source code helps, unfortunately `PlatformDetection` and `Latch` class are internal – Weifen Luo Nov 22 '21 at 03:20
  • That doesn't get you any closer to describing *why*. Why do you want a weak event at all? What are you using it for and why? Can you achieve that goal in a way that is more immediate / more deterministic? But even if you decide that you must use `WeakReference`, why are you trying to unit test it yourself? Can you just assume that the runtime has tests for that feature. – Jeremy Lakeman Nov 22 '21 at 03:24
  • Then that story can be a little bit long. I'm developing a framework providing class `Entity` and `Record` similar to `System.Data.DataRow` and `System.Data.DataRowView`, with the most significance that the prior one is strongly typed. `Entity` is the model provides its own event to notify changes, `Record` is a viewmodel that wraps `Entity` to translate the change event into `INotifyPropertyChanged` plus other things such as databinding, validations, etc. `Entity` should not aware the existence of `Record`. – Weifen Luo Nov 22 '21 at 03:46
  • The weak event is to prevent memory leak: `Record` should be GC reclaimed when only `Entity` is still referenced. – Weifen Luo Nov 22 '21 at 03:48
  • If you are going to add more context, update the question. – Jeremy Lakeman Nov 22 '21 at 03:48
  • Update the question may get things unnecessarily complicated IMHO. – Weifen Luo Nov 22 '21 at 03:50
  • @WeifenLuo Could you give us a minimal example of the event hookup that you're trying to perform that requires this weak reference? We may be able to provide a better alternative to a `WeakReference`. Also keep in mind, if your runtime logic relies on this reference, and you're not able to deterministically unit test it - you should comment the applicable areas appropriately with why you chose that logic and maybe some info on how you can prove that it works logically, although non-unit testable. – DekuDesu Nov 22 '21 at 03:52
  • 1
    @DekuDesu I've updated the question with the event hookup example. – Weifen Luo Nov 22 '21 at 04:07
  • 1
    The common pattern for event registration, is to implement `IDisposable` to remove your event handlers. Making it the callers responsibility to manage the lifetime of your object and dispose it. You seem to be using `WeakReference` in an attempt to invert that design pattern, I assume so that you don't have to bother disposing your `IEntityListener` implementations. – Jeremy Lakeman Nov 22 '21 at 04:11
  • 1
    I did consider the `IDisposable` to remove the event handlers. The problem is `Record` (which implements `IEntityListener`) is the view model, it should be disposed automatically when UI is disposed. I can't assume that because some UI framework such as WPF, does not provide `IDisposable` for UI elements. Please keep in mind I'm developing a framework, not an application. It should has minimum requirement for users. – Weifen Luo Nov 22 '21 at 04:19
  • The problem then is that your viewmodel needs to be disposed, and you shouldn't count on the UI to do it. A proper MVVM setup would have hooks outside of the view to know when the view has been dismissed, and the VM could be disposed at that point. @JeremyLakeman is correct: you are incorrectly inverting the design pattern here, and your code is screaming this fact at you. I wouldn't use **any** "framework" that relies on finalizers to unhook events. Full stop. I like my app not to crash for no reason when the finalizer thread is murdered by a NullReferenceException. – aaronburro May 17 '22 at 17:42

3 Answers3

1

Now that you've explained the reason you are trying to use WeakReference. The solution you really need is to implement IDisposable. This gives the caller precise control of the lifetime of your classes, allowing the garbage collector to release resources as soon as they are unused.

I would consider the use of WeakReference to avoid IDisposable an anti-pattern. Garbage collection is an expensive operation. Each version of the .net runtime may tweak how the garbage collector works, how often it occurs, and what garbage is removed. Until that collection occurs, every instance of IEntityListener will continue to receive events after it's lifetime has been completed. Forcing any implementation to require extra guard code to prevent mis-behaving.

Though implementing IDisposable has an annoying tendency to spread throughout every class. The tradeoff is worth it. Even if you are building a framework for others to reuse. I don't believe that using IDisposable as part of the contract between client code and framework would dissuade anyone from using your library. However, attempting to use WeakReference to invert this contract, might give your library a negative reputation.

Jeremy Lakeman
  • 9,515
  • 25
  • 29
  • Thank you very much for your answer. Instead of consider the use of `WeakReference` to avoid `IDisposable` an anti-pattern, I would rather consider it a tradeoff between performance and ease of use. WPF has a `WeakEventManager` to do this exactly, which I believe it uses `WeakReference` internally. – Weifen Luo Nov 22 '21 at 05:04
  • WPF has a ```WeakEventManager``` because it has non-deterministic resource releasing, not because there is a tradeoff in performance or ease of use. Microsoft is **very** clear about when to use the ```IDisposable``` pattern verses when to use finalizers. It sounds like you need to read up on this topic before you force a "framework" with extremely dangerous code in it onto other people. – aaronburro May 17 '22 at 17:44
0

The reason this fails in DEBUG mode is explained in my answer to https://stackoverflow.com/a/58662332/1261844 (which you linked to). If you inspect the IL generated by your code, you will see that the compiler generated a reference to the object you created in the very first line in a temporary variable (and also possibly when you create the WeakReference). That temporary variable is rooting your object. When optimizations are turned on, the compiler generates different IL which avoids the use of temporary variables where possible.

Put simply, if you want to test something like this, you MUST create the object AND its WeakReference in another method. Not coincidentally, that's also what the code in your suggested answer (which is still wrong, btw) is doing.

Jermey Lakemen's answer is also correct. You shouldn't be using a finalizer to unhook an event. Managed resources (such as unhooking events) should always be cleaned up in a Dispose() method. Finalizers are for unmanaged resources. There's very real reasons for why using a finalizer to do this a terrible idea, not the least of which being that you can't guarantee that your class still has valid references inside of it when it's being finalized. Using a WeakEventManager is a last resort for when you don't know when an object will actually be free, and even then, that's usually a code smell that you are doing something wrong.

aaronburro
  • 504
  • 6
  • 15
  • I tried to create the object AND its WeakReference in another method, it's still un-deterministic. The finalizer only exists in testing code, which is not distributed with the runtime, it's perfectly okay IMO. Weak event pattern (https://learn.microsoft.com/en-us/dotnet/desktop/wpf/events/weak-event-patterns) is a viable pattern, it's not something we should avoid at all cost. – Weifen Luo May 18 '22 at 06:16
  • What, exactly, failed? All of my unit tests around memory leaks rely on this, and not a one fails in debug or release. Also, you are conflating Weak Events and finalizers in this conversation. I'm saying not to use a finalizer to unhook any kind of event, because it's a huge code smell. It's also massively dangerous. You are writing a framework of some sort that others will presumably use. In the process, you are making a *major* memory management mistake, one which can and will crash the application without warning. You shouldn't he musings finalizer if you arent totally clear on how It works – aaronburro May 19 '22 at 23:52
  • The second code smell is you running straight to a Weak Event pattern. That's a last resort, not a first resort. There are places for it, sure, but something like that almost always indicates that you are inverting object creation patterns. You should be looking for that and fixing it. I can count on one hand the times I've needed weak events. And every single time was a problem with objects being created poorly, but it was too hard to change in a massive enterprise application. This is your framework, where you are in charge of making objects. You can and should fix that problem. – aaronburro May 19 '22 at 23:57
  • Yes, MS uses a WeakEvent pattern in WPF. But those guys are way smarter than you and I combined, times about 500. I trust Eric Lippert to know when it absolutely has to be used. Me and you? Not so much. – aaronburro May 19 '22 at 23:58
  • In the meantime, you should be fixing the object lifetime issue so you can properly unhook events without risking crashing the Finalizer thread. And if you don't know what the Finalizer thread is, then you shouldn't even be using it. – aaronburro May 20 '22 at 00:01
  • 1
    First of all, I didn't unwire any event in the finalizer. Instead I unwire the event when the target of the weak reference is garbage collected. Please read "WeakEntityListner to do the event hookup:" source code, particularly the call to the `CleanUp` method. – Weifen Luo May 20 '22 at 07:35
  • I use the finalizer only to tell whether the target of weak reference is garbage collected, to mock the condition that will trigger the event unwire. I hope you can understand the difference between the runtime and unit testing code. – Weifen Luo May 20 '22 at 07:39
  • It's good to know your finding regarding creating the object AND its WeakReference in another method, but it's an undocumented implementation detail, which does not guarantee a precise GC. Actually there is no way to guarantee a precise GC, because it un-deterministic by design. I prefer to use finalizer in the testing code, because it does not rely on any implementation detail. – Weifen Luo May 20 '22 at 07:46
  • Weak event pattern maybe something big for you, but not for me. Please do NOT combine you and me together. – Weifen Luo May 20 '22 at 07:49
  • 1
    Trust me, I fully understand your point. Usually I don't downvote any answer for courtesy - actually this is my only downvote so far, I even got a new "critic" badge for that! – Weifen Luo May 20 '22 at 08:05
  • Heh, congrats on that new badge :D By using a finalizer here, you actually *are* relying upon an implementation detail, just a different one. I think I see what your issue here was. You will need to create *both* your IEventListener instance *and* your WeakEntityListener in another method and return them back to your calling test method in order to have any shot at testing this scenario. Also, try passing GCCollectionMode.Forced into your call to GC.Collect(). In newer version of .NET, since at least 4.6.2, that is respected, and will remove the non-deterministic portions of it. – aaronburro May 20 '22 at 19:19
  • What is happening under the hood is that, in DEBUG mode, the compiler is generating local IL variables to store items into before it passes them in to functions you are calling. Those variables are "invisible" to you, but very real to the CLR. That's likely what is rooting your objects. Not using GCCollectionMode.Forced is what is causing the non-deterministic behaviour. – aaronburro May 20 '22 at 19:22
  • As for your proposed solution using a finalizer, you don't want to do that, because the GC might NULL out your reference to Latch before the code in the finalizer gets called. It's not well defined how that works. – aaronburro May 20 '22 at 19:25
-1

I ended up with the following solution, inspired by runtime WeakReference unit test source code suggested by @Jeremy Lakeman (https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/tests/System/WeakReferenceTests.cs)

using System;
using Xunit;
using Xunit.Abstractions;

public class WeakReferenceTests
{
    private sealed class Latch
    {
        public bool FinalizerRan { get; set; }
    }

    private sealed class TestObject
    {
        ~TestObject()
        {
            Latch.FinalizerRan = true;
        }

        public Latch Latch { get; } = new();
    }

    private readonly ITestOutputHelper output;

    public WeakReferenceTests(ITestOutputHelper output)
    {
        this.output = output;
    }

    [Fact]
    public void Test1()
    {
        TestObject realObject = new TestObject();
        Latch l = realObject.Latch;
        Assert.False(l.FinalizerRan);
        var weakReference = new WeakReference(realObject);
        Assert.NotNull(weakReference.Target);
        GC.KeepAlive(realObject);
        realObject = null;
        GC.Collect();
        GC.WaitForPendingFinalizers();
        GC.Collect();
        if (!l.FinalizerRan)
            output.WriteLine("Attempted GC but could not force test object to finalize. Test skipped.");
        else
            Assert.Null(weakReference.Target);
    }
}
Weifen Luo
  • 603
  • 5
  • 13
  • See my answer. This still may fail in debug mode sporadically. It's also very dangerous to follow references inside a Finalizer, because they are not guaranteed to be valid. You might also want to check out the .NET documentation for finalizers (https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/finalizers). It suggests there are changes in .NET 5 to how they work, which is probably the source of your issues. – aaronburro May 17 '22 at 17:36