pc » Code Review

WeakAction, or delegates with a weak reference to their target

admin — Mon, 11 Apr 2011 02:54:51 +0000

Abstract: Thoughts on implementing an Action-type delegate that holds only a weak reference to the target object.

Background

I recently started evaluating MVVM libraries for my client’s project. Though I often share parts of my own unpublished library with a client I had inherited this project and it already had it’s share of commonly-used MVVM types. What it lacked, and I added, was a quickie "message bus" implementation. Not the distributed, enterprise-class sort of message bus, but an intra-app messaging service that decouples senders from receivers. It’s the message bus that I really wanted to replace with a more robust, tested version that must surely already exist in one of the established MVVM libraries.

As luck would have it the first library I downloaded touted a messaging service but, as is too often the case, you get what you pay for. After perusing the code for a bit I could see that the messaging functionality wasn’t thread-safe, a feature I require. My app is a multi-threaded WPF app with both managed and native threads and takes pains to keep work off the UI thread in order to achieve smooth and somewhat real-time rendering of data. Wrapping the library’s messaging code for synchronization would be easy, but it doesn’t feel like fine enough granularity of locking for good performance.

Weak Action is Weak

Another driver in looking for an established library is that my quickie message bus implementation holds references to subscribed listeners. Not really a problem unless a listener fails to unsubscribe, in this case the listener will never be garbage collected. I consider that an undesirable situation. So I took a look at the above library’s implementation of a weak action; I quickly saw that this code used not a weak reference but a strong reference; it suffered the very problem it purported to fix. FYI, I did verify that the problem is known to the library’s author.

Now, at this time I haven’t finished evaluating libraries, and this was a bit of a rough start. Maybe I’ll just set aside some time to come up with a solution myself. Like… now.

An Elegant Solution (maybe…)

The problem I’d like to solve is that Action holds a reference to the target object of non-static delegates, preventing that target from being GC’d. The most elegant solution would allow me to simply replace my use of Action and Action with an implementation that holds only a weak reference to the target. And here it is:

static Action CreateWeakAction(Action action)
{
    if (action == null)
        throw new ArgumentNullException("action");
  
    // If it's a static delegate there's no need to create a weak reference.
    if (action.Target == null)
        return action;
  
    Type type = action.GetType();
    WeakReference targetRef = new WeakReference(action.Target);
    MethodInfo method = action.Method;
  
    return () =>
        {
            // Don't reference 'action' in this lambda expression.
            object target = targetRef.Target;
            if (target == null)
            {
                // Target was GC'd; don't do anything. (Ick.)
            }
            else
            {
                Delegate.CreateDelegate(type, target, method).DynamicInvoke();
            }
        };
}

Using CreateWeakAction is pretty easy:

Action action = CreateWeakAction(() => receiver.DoSomething(42));
// Time passes...
action();

Now the target (‘receiver’ in the above example) is free to be collected, but I have a new Action delegate returned from CreateWeakAction that will not be collected unless I release my reference to it. More than that, I have no way of telling whether the delegate I’m holding no longer has a live reference to the target. I could conceivably change my Action to a Func<> that returns an indicator of whether the target was GC’d, but that would require me to actually call the delegate to determine its state. And I would lose the elegant drop-in replacement I was hoping for.

A More Measured Solution

Setting aside my personal desire for elegance solutions, the actual requirements I’m looking to satisfy in my message bus are twofold: 1) don’t hold a reference to the target listener and 2) don’t hold unnecessary objects in the heap. A weak action implementation that satisfies these needs might look something like this:

class WeakAction
{
    public WeakAction(Action action);
    public bool IsTargetAlive { get; }
    public WeakActionInvocationResult Invoke();
}

This would allow me to create an instance of my weak action, invoke it’s behavior when needed, and check whether the target is still alive so that I can cull stale instances from my message bus implementation. And, thanks to an uninterrupted morning at my local coffee joint I have what I wanted. You may note that I’ve made an abstraction for invoking instance delegates; this makes that code more easily reusable for generic implementations (e.g., WeakAction).

internal class WeakAction
{
    private readonly InvocationAgent _instanceAction;
    private readonly Action _staticAction;
  
    public WeakAction(Action action)
    {
        if (action == null)
            throw new ArgumentNullException("action");
  
        if (action.Target != null)
        {
            _instanceAction = new InvocationAgent(action);
        }
        else
        {
            _staticAction = action;
        }
  
        Debug.Assert(_instanceAction == null || _staticAction == null);
        Debug.Assert(!(_instanceAction == null && _staticAction == null));
    }
  
    public bool IsTargetAlive { get { return IsStatic || _instanceAction.IsTargetAlive; } }
  
    private bool IsStatic { get { return _staticAction != null; } }
  
    public WeakActionInvocationResult Invoke()
    {
        if (_instanceAction != null)
            return _instanceAction.Invoke();
  
        _staticAction();
        return WeakActionInvocationResult.Invoked;
    }
}

Note that I’ve made an optimization for static delegates—there’s no need to incur the cost of dynamic invocation in that case because a static delegate has no target, therefore it won’t prevent anything from being GC’d.

Now imagine we have a messaging implementation that needs to track listeners with Action delegates but would rather not keep a reference to listeners forever if they happen not to unsubscribe. We can now create an instance of WeakAction and use it to invoke the listener’s Action when we wish; we can also query the WeakAction on occasion to determine whether it still has a live listener.

Finally here are the missing parts to make it work, the interesting bits. What makes this work is pulling apart the delegate into its component parts, holding the target part with a weak reference, and reassembling the parts into a delegate only when needed to invoke the original delegate. No reference to the original delegate is held.

internal enum WeakActionInvocationResult
{
    Invoked,
    Collected,
}
  
internal class InvocationAgent
{
    private readonly Type _type;
    private readonly WeakReference _targetRef;
    private readonly MethodInfo _method;
  
    public InvocationAgent(Delegate action)
    {
        Debug.Assert(action.Target != null, "Expected a non-static delegate");
  
        _type = action.GetType();
        _targetRef = new WeakReference(action.Target);
        _method = action.Method;
    }
  
    public bool IsTargetAlive { get { return _targetRef.IsAlive; } }
  
    public WeakActionInvocationResult Invoke(params object[] args)
    {
        object target = _targetRef.Target;
        if (target == null)
            return WeakActionInvocationResult.Collected;
  
        // Don't keep a reference to this delegate.
        Delegate.CreateDelegate(_type, target, _method).DynamicInvoke(args);
        return WeakActionInvocationResult.Invoked;
    }
}

I’ll leave it as an interesting challenge to the reader to implement WeakAction. Here’s what it might look like:

class WeakAction
{
    public WeakAction(Action action);
    public bool IsTargetAlive { get; }
    public WeakActionInvocationResult Invoke(T t);
}

Grab a cup of coffee and have at it. Please note: I’ve tested only on the desktop CLR, not Silverlight or Phone, so caveat emptor.

Where Did My Object Go? Part 2

admin — Mon, 03 Dec 2007 20:18:00 +0000

In Part 1 of this article I discussed the possibility of an object instance being collected before a method returns in this scenario:

    new MyObject().LongRunningMethod();

as well as this scenario:

    MyObject o = new MyObject();    o.LongRunningMethod();

Here we’ll discuss how this could become a problem. Frankly, it’s pretty easy to cause the problem, but I think it generally involves some ugliness on the part of software design or implementation, largely involving clean-up of fields when the instance is finalized.

Don’t Try This at Home

I don’t think it’s likely you’ll see a lot of code that does this, but here’s one way to run afoul of your object going way: export a reference to a field that the object cleans up when it’s finalized. You can export the reference, say, by making that field visible through a property. Exposing a field that you’re going to clean up in the finalizer would be an easy way to create a coupling between your object and other (arbitrary) client code that has no knowledge of your object’s life span.

Call Stack Antics

Another way to invoke the potential problem is to lose your “this” reference on the call stack. The code below manages to lose the “this” reference by passing a field to the helper method rather than allowing the helper to access the field directly via its own “this” reference. When the helper tries to access fs.Length an ObjectDisposedException is thrown.

Why does this throw? Well, the last live reference to the instance was lost when LongRunningMethod passed _input to Helper. Essentially we’ve again exported the field value from the instance and no longer hold a reference to the instance, allowing the GC to finalize it. Helper is left holding a reference to an object that has been finalized.

Note: Again, you will not see this behavior in a debug build. When running code marked as “debug” the JITter extends the lifetime of the local object to the end of the method. So you will not see this effect if you’ve compiled with the /debug flag.

    using System;    using System.IO;

    // Ugliness ensues.
    sealed class MyUglyObject
    {
        public MyUglyObject(string inputPath)
        {
            // Real production code would likely not delete the file when done...
            // ...but this is a sample app.
            _input = new FileStream(Path.GetTempFileName(), FileMode.Open, FileAccess.Read, FileShare.Read);
        }

        ~MyUglyObject()
        {
            if (_input != null)
            {
                _input.Close();
                _input = null;
            }
        }

        public void LongRunningMethod()
        {
            Helper(_input); // Our last reference to 'this' (implicit).
        }

        private void Helper(FileStream fs)
        {
            // A long-running method can easily experience a garbage collection
            // before returning. This one happens for force it to occur.
            GC.Collect();
            GC.WaitForPendingFinalizers();

            // Ka-boom!
            long inputSize = fs.Length;
            // ...
        }

        private FileStream _input;
    }

    class Program
    {
        static void Main(string[] args)
        {
            new MyUglyObject(@"....readme.txt").LongRunningMethod();
        }
    }

Can You See It?

As you can see above it takes a bit of effort to cause the code to blow up. If Helper had used _input.Length instead of taking a parameter the problem would not exist.

But, what I find a bit creepy about the above code is that if Helper were a static method it would seem respectable:

    private static void Helper(FileStream fs)
    {
        // A long-running method can easily experience a garbage collection
        // before returning. This one happens for force it to occur.
        GC.Collect();
        GC.WaitForPendingFinalizers();

        // Ka-boom!
        long inputSize = fs.Length;
        // ...
    }

At first glance it now looks like helper is a normal static helper function, as is likely seen in code bases across the world. It doesn’t need a “this” reference, it takes a reference to the object it uses, everything appears fine on the surface. Would you see the “this” reference being lost by the code calling Helper in a code review? I’m not so sure I would have until recently.

Technorati tags: Coding, Software Design, CLR, C#, Code Review

Where Did My Object Go? Part 1

admin — Mon, 03 Dec 2007 19:14:00 +0000

I ran across this scenario a few months ago and was just reminded of it. It takes a bit of an edge case to make it a problem, but it’s interesting all the same.

There’s a regular idiom in C# in which we call a method on an object instance that we’ve created inline:

    new MyObject().LongRunningMethod();

There assumption may be an assumption that the lifetime of this instance of MyObject extends at least until LongRunningMethod returns, but this isn’t necessarily true. This same assumption is often made about local references to objects:

    MyObject o = new MyObject();    o.LongRunningMethod();

However, in both these cases the object instance may be collected before LongRunningMethod returns.

Does this really happen?

Yes, it can and does. The code below exercises this behavior. When you run it you will see the following output, indicating that the object was collected and finalized before LongRunningMethod returns:

    Using release build
    Inline
    Entering MyObject1.LongRunningMethod().
    Finalizing in ~MyObject1().
    Returning from MyObject1.LongRunningMethod().     Local reference
    Entering MyObject1.LongRunningMethod().
    Finalizing in ~MyObject1().
    Returning from MyObject1.LongRunningMethod().Note: You will not see this behavior in a debug build. When running code marked as “debug” the JITter extends the lifetime of the local object to the end of the method. So you will not see this effect if you’ve compiled with the /debug flag.

Note also that this article also assumes we’re using CLR 2.0. Future versions could obviously behavior differently.

Here’s the code. Just drop it into test.cs, run csc test.cs, and execute test.exe.

    using System;

    sealed class MyObject
    {
        ~MyObject()
        {
            Console.WriteLine("Finalizing in ~MyObject1().");
        }

        public void LongRunningMethod()
        {
            Console.WriteLine("Entering MyObject1.LongRunningMethod().");

            // A long-running method can easily experience a garbage collection
            // before returning. This one happens for force it to occur.
            GC.Collect();
            GC.WaitForPendingFinalizers();

            Console.WriteLine("Returning from MyObject1.LongRunningMethod().");
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
    #if DEBUG
            string build = "debug";
    #else
            string build = "release";
    #endif
            Console.WriteLine("Using {0} build", build);

            // Try it both ways.

            Console.WriteLine("Inline");
            new MyObject().LongRunningMethod();

            Console.WriteLine();
            Console.WriteLine("Local reference");
            MyObject o = new MyObject();
            o.LongRunningMethod();
        }
    }

Is this a problem?

Generally, I’d say it’s not a problem. Once it has begun execution, LongRunningMethod doesn’t need the original object reference unless it’s making reference to that instance. In that case the GC won’t be able to collect the object.

I’ll discuss how to make it a problem in Part 2 of this article.

Technorati tags: Coding, Software Design, CLR, C#

Say (code) what you mean

admin — Thu, 01 Feb 2007 16:26:00 +0000

A bright, shiny object caught my eye few weeks ago while I was hiking in the snowy Cascades. Upon investigation I found a saucer shaped craft glistening in the sun. It appeared to be a space travel-capable sort of vehicle with a translucent bubble-shaped canopy that had sprung open, apparently on impact with this planet. I peered into the craft and saw a gray-green being with a large head and eyes. This being appeared to be well-frozen and so, concluding that continuing my investigation would not disturb this being, I continued to poke around the strange craft a bit more.

In what I would describe as a cockpit I found a something like a display panel which was flashing this message:

Exception in thread “nav” java.lang.ClassCastException: java.lang.Double
at NavigationWaypoint.(NavigationWaypoint.java:10)
at FindWaypoint.FindNextWaypoint(FindWaypoint.java:94)
…

Obviously this sparked my curiosity! In looking around the cockpit I discovered a bit of paper–more like mylar, really–clutched in what for now I’ll call the unfortunate being’s hand. I liberated this piece of paper and found printing on it. I was shocked to see what the paper showed:

    7 class NavigationWaypoint
    8 {
    9     public NavigationWaypoint(Object name) {
   10         this.name = (String)name;
   11     }
   12 
   13     public String getName() {
   14         return name;
   15     }
   16 
   …
   32     private String name;
   33 }

What a tragedy! Clearly the author of this code (presumably an inexperienced alien programmer) had errantly designed the instance constructor to take an Object rather than a String. In doing so the coder then needed a typecast in order to coerce the Object to a String in order to set the name field.

I don’t know about most, but on my planet such use of a typecast is a pretty strong clue that the programmer should look around to see why the typecast is necessary; it may be indicative of a design error.

In fact, in this case it is a design error. The NavigationWaypoint class has a name field that contains a string, yet the constructor allows the caller to pass in an object of any type. This unfortunate error and the use of a typecast caused the error to manifest itself during execution of the code; obviously some other code passed in a Double rather than a String, probably a simple coding error. If the constructor had been coded to accept a String this coding error would have been caught at compile time rather than during execution, saving this little fellow the exasperation of debugging on the fly and waking up frozen on a strange planet. If only the constructor had been coded to correctly represent the type’s data….

(This post was inspired by a true story that had nothing to do with navigation.)

Technorati tags: Coding, Design, Development, Software

Red Flag

admin — Fri, 25 Aug 2006 06:15:00 +0000

A red flag. It’s a warning. An alert. An indication of danger. A notification that something is amiss. There are red flags in the code we work on and the processes we follow. But do we see them? I missed a red flag recently. It happened like this:

I had this curious bug I was trying to fix. The behavior suggested that it was most likely corrupted or uninitialized memory. That’s what intuition borne of experience was telling me, anyway. Randomly timed incorrect behavior in code that was processing a static stream of data. The input data was constant from one run to the next, the bits flowing through the code always the same, but the end result varied pretty much randomly in where and when it failed.

This suggested to me that we were processing someone else’s data or uninitialized data (which is really just someone else’s data from within the same process).

This body of C++ code was unfamiliar to me, so I found myself picking the brains of a coworker who had been around a while. In discussing the bug I found myself looking over his shoulder as he scrolled through some of the code in question, and he commented on a variable assignment that wasn’t used later in the function.

It was one of those pfft moments. “Been there, done that, seen it a million times.” A thoughtless assignment statement that someone typed in but then lost their train of thought. It looked something like this:

void fn()
{
    size_t cbBase;
    void* pvData;

    if (get_value("base", &cbBase, &pvData))
    {
        store_data("base", cbBase, pvData);

        size_t cbExtended;
        void* pvDataExtended;

        if (get_value("extended", &cbExtended, &pvDataExtended))
        {
            store_data("extended", cbExtended, pvDataExtended);
            cbBase = cbExtended;
        }
    }
}

And quickly we moved on to discuss what might really be wrong with the code. And that quickly I’d dismissed the red flag.

In a world where most of the code that I interact with is not my own, where dozens of changes wrought by numerous hands happen over a period of years can I really pass off a small, unexplained assignment like that above as an innocuous error? Any moderately complex code base will transmogrify over the years. Initial errors may indeed be simple coding issues that we wish would have been corrected by code review, but over time source code changes not randomly but with specific intent. And with any luck you have both bug reports and a source code revision system on which you can rely to find that intent.

The red flag, of course, was the meaningless assignment statement. More than a day later as I waded through diffs of check-ins from ages past I ran across the rationale for the assignment. In previous check-in an attempt was made to correct some bad behavior. A previous version of the code looked more like this:

void fn()
{
    size_t cbBase;
    void* pvData;

    if (get_value("base", &cbBase, &pvData))
    {
        store_data("base", cbBase, pvData);

        size_t cbExtended;
        void* pvDataExtended;

        if (get_value("extended", &cbExtended, &pvDataExtended))
        {
            store_data("extended", cbExtended, pvDataExtended);
            cbBase = cbExtended;
        }

        if (cbBase < MINIMUM_EXPECTED_DATA_SIZE)
        {
            backfill_missing_extended_data();
        }
    }
}

Ah, the unexplained assignment was orphaned by a previous check-in. In an effort to correct a particular problem a developer had removed code but left behind an ineffective assignment. Interestingly–partly because I like a tidy ending–the bug the developer was fixing was strongly related to the bug I was pursuing. The original author’s intent for the assignment, it turns out, was probably not

    cbBase = cbExtended;

But

    cbBase += cbExtended;

I reintroduced the missing code and patched up the assignment to find that, very conveniently, my bug was fixed as well. In the end, yes, it was incorrectly initialized data. It just wasn’t where I expected to look.

Funny thing, those red flags. They’re hard to see. Where have you seen them lately? (Or not?)