what is the counter example for this code generating a modified closure warning that would cause a problem?

Question

There are other questions on SO about this but it's my first time and I'm failing to completely comprehend.

QUESTION 1: Assuming this example is actually safe, i.e. the warning could be ignored: How would this code need to be changed so that the problem this warning warns about will be real?

QUESTION 2: How is it that the fix applied for the warning makes the warning go away? My intuition tells me it is the same result.

Here's the code:

    public static void SynchCreativesForCampaign(int pid, ILogger logger)
    {
        var db = new SynchDbDataContext(true);

        foreach (var creativeItem in CreativeList.Create(pid).CreativeItems)
        {
            logger.Log(@"creative id " + creativeItem.CreativeId);

            var creativeDetail = CreativeDetail.Create(creativeItem.CreativeId);

            //var item = creativeItem; <-- this gets added by the "fix" for the warning
            var creativeEntity = (from c in db.CreativeEntities
                                  where c.dtid == creativeItem.CreativeId
                                  select c).FirstOrDefault();

            if (creativeEntity == null)
            {
                creativeEntity = new CreativeEntity {dtid = item.CreativeId};
                db.CreativeEntities.InsertOnSubmit(creativeEntity);
            }
        }

        db.SubmitChanges();
    }

Here's the warning:

enter image description here

I can't remember how to reproduce the problem. The problem is that under some circumstances, a closure will capture a local variable, and retain the original value of the variable, even if the local is later changed. — John Saunders, Mar 24 '11 at 01:45
possible duplicate of [Access to Modified Closure](http://stackoverflow.com/questions/235455/access-to-modified-closure) — John Saunders, Mar 24 '11 at 02:31
found it: http://stackoverflow.com/questions/235455/access-to-modified-closure — John Saunders, Mar 24 '11 at 02:32

score 0 · Answer 1 · answered Mar 24 '11 at 01:43

Here's how it might bite you. The example is a bit contrived, but you will get the idea.

public static void SynchCreativesForCampaign(int pid, ILogger logger)
{
    var db = new SynchDbDataContext(true);
    CreativeEntity creativeEntity = null; // NEW: pull this out of the loop

    foreach (var creativeItem in CreativeList.Create(pid).CreativeItems)
    {
        logger.Log(@"creative id " + creativeItem.CreativeId);

        var creativeDetail = CreativeDetail.Create(creativeItem.CreativeId);

        if (creativeEntity != null) {
            continue; // NEW: let the loop go on, but make sure we only
                      // wrote to creativeEntity *once*
        }

        // IMPORTANT: notice I don't immediately call FirstOrDefault!
        creativeEntity = from c in db.CreativeEntities
                         where c.dtid == creativeItem.CreativeId
                         select c;

    }

    if (creativeEntity != null)
    {
        // Only NOW evaluate the query
        var result = creativeEntity.FirstOrDefault();

        // OK, stop: result holds the creative entity with the dtid
        // referring to which CreativeItem.CreativeId?
    }
}

You might answer "with the dtid referring to the first item in the CreativeItems collection that we looped over", but the reality is that it's going to refer to the last one (this is the reason I let the loop continue -- it did nothing except change the value of creativeItem, which was necessary to let the bug surface).

The code works by doing some compiler voodoo which in essence extends the lifetime of the creativeItem variable until the point where you evaluate the query. So even though it looks like that after the foreach ends creativeItem no longer exists, in actuality it's waiting somewhere in memory and of course its value remains unchanged from what it had during the last iteration.

Now consider what happens if you copy the value of creativeItem to another variable with scope only inside the foreach loop. Now that variable's lifetime is extended, but there is an important difference: that variable, being scoped inside the foreach, will not get reused for multiple iterations. In each iteration, a new variable (with the same name of course) will be used, and it will be that value you will see when evaluating the query.

what is the counter example for this code generating a modified closure warning that would cause a problem?

1 Answers1