Peter Ritchie's MVP Blog : C# 3.0, .NET 3.5

Deep Dive on Closure Pitfalls

PeterRitchie — Wed, 03 Nov 2010 13:23:00 GMT

I've blogged about closures in C# and their pitfalls before. I keep seeing problems with closures--more now that lambdas expressions and statements ("lambdas") are becoming more widespread--even with experienced developers. So, I'd thought i'd post about some of the details surrounding where the C# compiler generates closures in the hopes that people will recognize more where they write code that creates a closure and its context.

The C# language spec does not refer specifically to "closures", with regard to capturing state for anonymous methods (including lambdas)--it refers to "outer variables" and "captured outer variables". The captured outer variables for a specific anonymous method are the "closure".

The compiler captures the state of variables within the local scope of the anonymous method in a compiler-generated class. The compiler effectively also captures the declaration scope of the state within the closure. If a variable is declared outside the local scope, that is echoed in the use of that closure. If the variable is declared within the local scope, that is captured in use of the closure. So, an anonymous method that uses state (a variable) declared outside the local scope of the anonymous method, the compiler will generate a closure use that closure so that variable is only instantiated once. For example:

	ICollection<Func<string>> Funcs = new List<Func<string>>(); 
	String[] texts = new String[] { "one", "two", "three" };
	foreach (String text in texts)
	{
		Funcs.Add(delegate { return text; });
	}

We have an anonymous method used within the local scope of the body of the foreach loop. The anonymous method uses the text variable declared outside the scope of the foreach loop. The compiler generates a class to contain the state and the code that uses that state (i.e. the anonymous method)--otherwise the highlighted code--and make use of that generated class. For example:

	Closure closure = new Closure();
	for (int i = 0; i < texts.Length; ++i)
	{
		closure.text = texts[i];
		Funcs.Add(new Func<string>(closure.Func));
	}

The compiler-generated class may look something like this:

	internal class Closure
	{
		public string text;
		public string Func()
		{
			return text;
		}
	}

The compiler instantiates that class outside the generated for loop (because there is no "foreach" loop in .NET IL--what we see here for generated code is a C# view of IL) to match the single declaration of the text variable (as the body of the foreach loop sees it). So, as we can tell from this code, each of these Func delegates are accessing the same variable. So, if we invoked each of those delegate, they would all return "three". While the code looks like each Func delegate is dealing with a different value ("one", then "two", then "three"); it's not and you've probably got a logic error with this code.

So, as I pointed out in a previous post, we get around the problem by bringing the value from the outer scope into the inner scope of the anonymous method. For example:

	foreach (String text in texts)
	{
		var temp = text;
		Funcs.Add(delegate { return temp; });
	}

Now, the compiler needs to generate a class to model this local scope--or the highlighted code. This local scope includes the creation of an object and assigning it a specific value. So, what the compiler generates now would be similar to the following:

	for (int i = 0; i < texts.Length; ++i)
	{
		Closure closure = new Closure();
		closure.temp = texts[i];
		Funcs.Add(new Func<string>(closure.Func));
	}

With a closure class similar to:

	private class Closure
	{
		public string temp;
		public string Func()
		{
			return temp;
		}
	}

As we can see here, the compiler is now instantiating a Closure class once for each iteration of the loop, assigning a specific value to the temp field, then passing a delegate to the Func instance method to a new Func<T> delegate. Now, if we invoke each of our Func delegates we get what we expect "one", then "two", then "three".

I've specifically chosen deferred execution in these examples to separate parallel processing and what's going on. The same issue occurs when we use anonymous delegates with multiple threads. For example:

	var texts = new[] { "one", "two", "three" };
	foreach (var text in texts)
	{
		ThreadPool.QueueUserWorkItem(v => Trace.WriteLine(text));
	}

This anonymous method (as a lambda) would generate code similar to:

	Closure closure = new Closure();
	for (int i = 0; i < texts.Length; ++i)
	{
		closure.text = texts[i];
		ThreadPool.QueueUserWorkItem(new WaitCallBack(closure.WaitCallback));
	}

As you can see, each thread is actually dealing with a single variable (Closure.text). At worst case when each thread runs, it outputs "three"--otherwise it's undefined what you'll see. We fix that the same way we did previously by adding a variable into the local scope:

	foreach (var text in texts)
	{
		var temp = text;
		ThreadPool.QueueUserWorkItem(v => Trace.WriteLine(temp));
	}

This then generates something like this:

	for (int i = 0; i < texts.Length; ++i)
	{
		Closure closure = new Closure();
		closure.temp = texts[i];
		ThreadPool.QueueUserWorkItem(new WaitCallback(closure.WaitCallback));
	}

This means each thread is operating on an independent variable (the Closure.temp field of each instance of Closure that was instantiated).

I hope this makes it more clear what the compiler is doing when you use anonymous methods (including Lambdas).

DevTeach 2009 Vancouver

PeterRitchie — Thu, 26 Mar 2009 16:33:00 GMT

The schedule for DevTeach 2009 Vancouver has been announced (http://www.devteach.com/). There’s lots of great software development sessions from some of the leaders in our industry.

If you’re planning on improving yourself, this is the conference to go to. Not only can you attend excellent sessions; but you can hob-knob with the presenters and pick their brains.

If you have a friend or co-worker who’s interested, there’s a limited-time two-for-one offer for an even better price: http://www.devteach.com/Register.aspx

Nested Types

PeterRitchie — Tue, 15 Jul 2008 14:47:00 GMT

Recently Michael Features blogged about nested types. The title was almost "nested types considered harmful".

I don't agree. I don't agree that they're any more harmful than any other C# construct (except goto...). Nested types are like anything else in our tool-belt: they have a time and place and can be abused.

But, when to use them? Well, for the most part I agree with Michael, you should avoid them. But, there are times when they're simply the best solution in a given set of circumstances.

Let's look at asynchronous programming model (APM) in .NET.

        // Paraphrased from MSDN

        // Accept one client connection asynchronously.

        public static void DoBeginAcceptTcpClient(TcpListener

            listener)

            // Start to listen for connections from a client.

            Trace.WriteLine("Waiting for a connection...");

            // Accept the connection.

            // BeginAcceptSocket() creates the accepted socket.

            listener.BeginAcceptTcpClient(

                DoAcceptTcpClientCallback,

                listener);

        // Process the client connection.

        public static void DoAcceptTcpClientCallback(IAsyncResult ar)

            // Get the listener that handles the client request.

            TcpListener listener = (TcpListener)ar.AsyncState;

            // End the operation and display the received data on

            // the console.

            TcpClient client = listener.EndAcceptTcpClient(ar);

            // TODO: do something with client.

            // Process the connection here. (Add the client to a

            // server table, read data, etc.)

            Trace.WriteLine("Client connected completed");

In this simple scenario we are getting by with a state of simply a TcpListener object. In a more complex scenario, you'll likely also want a connection-specific queue, some sort of information about what to do after a connection, etc. While you can use existing types of have several collection instance fields to keep track of each of these things; you then have to introduce synchronization of those collections, managing the content of those collections, etc.--it's much easier and safer to send that information on the stack. One method I've tried is simply passing an Object collection as the state; but that quickly becomes hard to manage because of the lack of type-safety on the elements in the array (if I remove an element and replace it with another type, the compile can't know and I'll get a run-time error instead of a compile-time error). To get type safety I generally introduce a new type to aggregate all the types I need in this asynchronous callback. While this new type *could* be reusable by other classes; it likely isn't and I don't want to then be bound that that explicit contract I've signed by making the types publicly available. The only option of not making them publicly available is as private nested types. For example:

        private class AcceptTcpClientParameters

            public CommandQueue CommandQueue { get; private set; }

            public Command NextCommand { get; private set; }

            public TcpListener TcpListener { get; private set; }

            public AcceptTcpClientParameters(int commandQueue, int nextCommand, TcpListener tcpListener)

                CommandQueue = commandQueue;

                NextCommand = nextCommand;

                TcpListener = tcpListener;

        // Accept one client connection asynchronously.

        public static void DoBeginAcceptTcpClient(TcpListener

            listener, CommandQueue commandQueue, Command nextCommand)

            // Start to listen for connections from a client.

            Trace.WriteLine("Waiting for a connection...");

            // Accept the connection.

            // BeginAcceptSocket() creates the accepted socket.

            listener.BeginAcceptTcpClient(

                DoAcceptTcpClientCallback,

                new AcceptTcpClientParameters(commandQueue, nextCommand, listener));

        // Process the client connection.

        public static void DoAcceptTcpClientCallback(IAsyncResult ar)

            AcceptTcpClientParameters parameters = ar.AsyncState as AcceptTcpClientParameters;

            if(parameters == null) return;

            TcpClient client = parameters.TcpListener.EndAcceptTcpClient(ar);

            parameters.NextCommand.Process(parameters.CommandQueue, client);

I find this use of nested types to be more object-oriented (the needs of the DoAcceptTcpClientCallback are abstracted), more intention revealing, better implements Single Responsibility Principle (SRP), better separates concerns, more maintainable and more agile.

Now, to be clear; this is forced set of circumstances. You're using a library that implements the APM (right? You haven't implemented APM yourself...). But, that's my point--nested types are almost essential in a given set of circumstances.

Upcoming C# 3 Guidance From Microsoft

PeterRitchie — Thu, 13 Mar 2008 15:46:00 GMT

Mircea Trofin has some design guidelines with regard to some C# 3 language additions (that I assume will make it into a revised Framework Design Guidelines of some sort). They more less agree with the guidelines I published in Code Magazine a while ago. There are some slight differences:

Consider using extension methods in any of the following scenarios: to provide helper functionally relevant to every implementation of an interface... and,

Do define extension methods in the same namespace as the extended type, if the type is an interface, and if the extension methods are meant to be used in most or all cases. This applies to framework designers that are publishing interfaces but also want to publish callable methods that apply to all implementation of those interfaces. My article approaches the guidelines more from a non-framework designer. I do agree that extension methods to extend interfaces is very useful and is probably one of the most adept use of extension methods. Although, I think the wording of this guideline could use improvement.

Avoid defining extension methods on System.Object, unless absolutely necessary. Good advice.

Do not define extension methods pertaining to a feature in namespaces normally associated with other features. Instead define them in the namespace associated with the feature they belong to, or a namespace of it. This is really unclear (and seems to suggest contradicting the first guideline: Avoid frivolous use of the extension methods feature when defining methods on a new type. Use the canonical, language-specific means for defining type members). I'm assuming the jist of this is, as a framework designer, don't arbitrarily put extension methods in the namespace of the type the method applies to, consider putting the extension method in a more applicable namespace, if possible. For example, if you want to declare an extension method "Forward" for Telecom.INode implementations, putting the method in a "Routing" namespace would be better than arbitrarily putting it in the "Telecom" namespace. I've changed the Mircea's guidance slightly to use a interface in the example--which I think makes it more clear.

Mircea also includes Consider using extension methods in any of the following scenarios:... when object model considerations would dictate taking a dependency on some assuming but taking such a dependency would break dependency management rules. This means, should you need to add a method to a class but adding that method would create cyclic dependency or would cause a lower level assembly/class to be dependant on a higher level class, break the method out into another assembly as an extension method. Use this advice with caution; I would argue that if you think you need a method like this at all (even if implemented as an extension method), you likely have some design problems and should only be considered when revising a published framework, and not when creating a new framework.

Another tidbit of guidance that came about after I wrote the article and Mircea doesn't mention is that extension methods can make writing fluent interfaces much cleaner by separating the state management concern of supporting a fluent interface from the class that it applies to.

Thoughts? Any additional guidance you feel has been overlooked (with regard to extension methods and LINQ)?

CoDe Magazine Article.

PeterRitchie — Mon, 04 Feb 2008 18:31:00 GMT

The January/February edition of Code Magazine includes my article "C# 3.0 Syntax Additions-Design Guidelines".

Visual C# Developer Center Article

PeterRitchie — Thu, 03 Jan 2008 20:27:00 GMT

My latest Visual C# Developer Center Article Inferred Typing with Factory Methods as Extension Methods is now online.

New warning CS0809 in C# 3 (Visual Studio 2008)

PeterRitchie — Mon, 26 Nov 2007 20:31:00 GMT

There were several breaking changes (fixes) in C# 3 from C# 2. One is the ability to attribute a member override with ObsoleteAttribute without also attributing it the virtual member in the base class.

For example, the following will compile without error In C# 2 (Visual Studio 2005/.NET 2.0):

using System;

internal class BaseClass

    public virtual void Method()

internal class DerivedClass : BaseClass

    [Obsolete]

    public override void Method()

public static class Progam

    public static void Main()

        BaseClass baseClass = new DerivedClass();

        baseClass.Method();

This same code will generate a CS0809 warning with C# 3.

This change was done because, as you can see in the example, if DerivedClass.Method is accessed via a BaseClass reference, the compiler can't reliably warn you that you're in fact calling an Obsolete method.

In reality, even you you did call DerivedClass.Method() via a DerivedClass reference you still wouldn't get a warning. This may have something to do with a quirk in the way the compiler detects attributes with overrides and virtual methods; but I'm surmising.

The fix, if you have access to the base class, is to attribute bot the base and the derived class member with ObsoleteAttribute. If you don't have access to the base class, you'll have to live with or suppress the warning.