On closures and captured variables

A few days ago, on the project I’m working on, I’ve stumbled on an interesting bug – an example of why it pays off to learn the ‘deeper’ areas of C# language (or any other language).
Image copyright: Pavel Shlykov (Shutterstock)

Image copyright: Pavel Shlykov (Shutterstock)


Greatly simplified (and with the class names changed to protect the innocent 🙂 ), we had:
– a structure of orders and order lines/items, something pretty straightforward:

// ...
    public class Order
    {
        public Order()
        {
            Items = new List<OrderItem>();
        }

        public string Number { get; set; }
        // ... other fields

        public IList<OrderItem> Items { get; set; }
    }
// ...
    public class OrderItem
    {
        public int ItemId { get; set; }
        public string ProductName { get; set; }
        public decimal Price { get; set; }
        // ... other fields
    }

An order contains several lines.

For one reason or another, let’s say that we want to deep copy this structure to another class, OrderLineDto, that flattens the structure:

    public class OrderLineDto
    {
        public string OrderNumber { get; set; } // the parent order number
        // OrderItem attributes:
        public int ItemId { get; set; }
        public string ProductName { get; set; }
        public decimal Price { get; set; }
        // ... other fields
    }

Because OrderItem has several hundred properties (don’t ask me why 🙂 ), I’m using AutoMapper to simplify the mapping job.
We added a helper class that it’s supposed to keep the code nice and tidy:

public class OrderMapper
{
    private readonly Order _order;

    public OrderMapper(Order order)
    {
        _order = order;

        AutoMapper.Mapper.CreateMap<OrderItem, OrderLineDto>()
          .ForMember(orderLineDto => orderLineDto.OrderNumber, 
             config => config.MapFrom(sourceOrderItem => _order.Number));
     }

     public OrderLineDto GetLineDto(OrderItem orderItem)
     {
         var dtoLine 
           = AutoMapper.Mapper.Map<OrderItem, OrderLineDto>(orderItem);
         return dtoLine;
     }
}

The constructor gets the current Order instance, defined the mapping, and the GetLineDto method is doing the actual mapping from a OrderItem to a new OrderLineDto. Pretty simple..
Only for OrderLineDto.OrderNumber, we have to tell AutoMapper to take the value from the ‘parent’ _order.Number.

Let’s test it in a console application:

        static void Main()
        {
            var order1 = new Order {Number = "O1"};
            var orderItem1 = new OrderItem
            {
                ItemId = 1,
                ProductName = "Book 1",
                Price = 100
            };
            order1.Items.Add(orderItem1);

            var order2 = new Order {Number = "O2"};
            var orderItem2 = new OrderItem
            {                
                ItemId = 2,
                ProductName = "Book 2",
                Price = 200
            };
            order2.Items.Add(orderItem2);

            /////////////
            var orderMapper1 = new OrderMapper(order1);
            OrderLineDto dto1 = orderMapper1.GetLineDto(orderItem1);

            Console.WriteLine("\n\rItem 1 order number: {0}  == DTO 1 order number: {1}", 
                                order1.Number, dto1.OrderNumber);

            Console.WriteLine("Item 1 prod. name: {0}  == DTO 1 prod name: {1}", 
                                orderItem1.ProductName, dto1.ProductName);

            //////////////
            var orderMapper2 = new OrderMapper(order2);
            OrderLineDto dto2 = orderMapper2.GetLineDto(orderItem2);

            Console.WriteLine("\n\rItem 2 order number: {0}  == DTO 2 order number: {1}", 
                                order2.Number, dto2.OrderNumber);

            Console.WriteLine("Item 2 prod. name: {0}  == DTO 2 prod name: {1}", 
                                orderItem2.ProductName, dto2.ProductName);

            Console.ReadLine();
        }

I create 2 Order objects, each with one OrderItem, and for each OrderItem, I map it to a OrderLineDto object.
Finally, I compare the original and DTO properties to make sure they were copied properly.

However, the result is not the expected one:

Item 1 order number: O1  == DTO 1 order number: O1
Item 1 prod. name: Book 1  == DTO 1 prod name: Book 1

Item 2 order number: O2  == DTO 2 order number: O1
Item 2 prod. name: Book 2  == DTO 2 prod name: Book 2

Obviously, the 2’nd DTO object does not have the right order number (‘O2’), but the first one, ‘O1’.
Is AutoMapper broken? 🙂

No – the culprit is the was I’m defining the custom mapping for OrderNumber:

public class OrderMapper
{
    private readonly Order _order;

    public OrderMapper(Order order)
    {
        _order = order;

        AutoMapper.Mapper.CreateMap<OrderItem, OrderLineDto>()
          .ForMember(orderLineDto => orderLineDto.OrderNumber, 
             config => config.MapFrom(sourceOrderItem 
                                   => _order.Number));
    }
...
}

sourceOrderItem => _order.Number
is a lambda expression, but because _order field is referenced, a closure is created.
As for any closure, the _order variable instance is captured, and will be available each time the lambda expression is evaluated.

Nothing unexpected so far – the question is: which instance of _order?
The one from the moment OrderMapper constructor is called and the lambda expression is instantiated, right?
🙂
That was the intention, at least.
However, AutoMapper has the good habit of caching the mappings in a static field, for good performance reasons.
So even if we try to redefine the mapping for a certain type, the first mapping is used.
In our case, the mapping will allways use the lambda expression created during the first call to Mapper.CreateMap, when the first OrderMapper is instantiated, so the first _order instance is captured by the closure and always used when OrderNumber is mapped.

How to fix this? Quite easy: copy the OrderNumber directly in code and don’t use AutoMapper for such a simple task:

    public class OrderMapper
    {
        private readonly Order _order;

        public OrderMapper(Order order)
        {
            _order = order;

            AutoMapper.Mapper.CreateMap<OrderItem, OrderLineDto>();
            
            //.ForMember(orderLineDto => orderLineDto.OrderNumber, 
            //        config => config.MapFrom(sourceOrderItem => _order.Number));
        }

        public OrderLineDto GetLineDto(OrderItem orderItem)
        {
            var dtoLine = AutoMapper.Mapper.Map<OrderItem, OrderLineDto>(orderItem);
            dtoLine.OrderNumber = _order.Number;
            return dtoLine;
        }
    }

To make sure that such a ‘bug’ is not introduced again by mistake, we can move the call to AutoMapper.Mapper.CreateMap in a static constructor that won’t be able to access instance fields.

More on closures and a comparation with Java: http://csharpindepth.com/articles/chapter5/closures.aspx
or http://martinfowler.com/bliki/Lambda.html
or in JavaScript: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Closures

Advertisements
This entry was posted in .NET, C# and tagged , , , . Bookmark the permalink.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s