Access to Modified Closure

Access to Modified Closure

In this case, it's okay, since you are actually executing the delegate within the loop.

If you were saving the delegate and using it later, however, you'd find that all of the delegates would throw exceptions when trying to access files[i] - they're capturing the variable i rather than its value at the time of the delegates creation.

In short, it's something to be aware of as a potential trap, but in this case it doesn't hurt you.

See the bottom of this page for a more complex example where the results are counterintuitive.

How to mitigate Access to modified closure in cases where the delegate is called directly

Install the JetBrains.Annotations package with NuGet: https://www.nuget.org/packages/JetBrains.Annotations

Mark the passed in delegate with the InstantHandle attribute.

private void ConsumeConsume([InstantHandle] Consume c)
{
    c();
}

From InstantHandle's description:

Tells code analysis engine if the parameter is completely handled when the invoked method is on stack. If the parameter is a delegate, indicates that delegate is executed while the method is executed. If the parameter is an enumerable, indicates that it is enumerated while the method is executed.

Source: https://www.jetbrains.com/help/resharper/Reference__Code_Annotation_Attributes.html

If you don't want to add the whole package to your project, it's enough to just add the attribute yourself, although it's hacky in my opinion.

namespace JetBrains.Annotations
{
    [AttributeUsage(AttributeTargets.Parameter)]
    public class InstantHandleAttribute : Attribute { }
}

Can someone explain access to modified closure in C# in simple terms?

Here is a fairly good explanation.

A closure is created when you reference a variable in the body of a method from a delegate. Essentially, a class that contains a reference to the local variable is generated.

If the variable is constantly modified, when an external method calls the delegate, it may contain an unpredictable value, or even throw an exception.
For example, in an example like this:

foreach (string data in _dataList)
{
    DoStuff (() => data);
}

The method () => data is always going to be the same method. if you store it, you don't know what happens when it's eventually invoked -- what will the value of data be at the time? Will it even be valid? This is especially dangerous if you use yield return.

A simpler example, without an iterator, is:

var x = 5;
Action f = () => Console.WriteLine(x);
x = 76;
f();

Access to modified closure: ReSharper

I would suggest avoid using a ref parameter for this in the first place - it seems needlessly complicated to me. I'd rewrite DoAction as:

static string DoAction(string data, Action<string> action)
{
    data = data == null ? "Initialized Data" : data + "|";
    action(data);
    return data;
}

Then you can have:

data = DoAction(data, Console.WriteLine);

or if you want to use a lambda expression:

data = DoAction(data, txt => Console.WriteLine(txt));

You can make DoAction a void method if you don't actually need the result afterwards. (It's not clear why you need the result to be returned and a delegate to execute in DoAction, but presumably that makes more sense in your wider context.)

Access to Modified Closure (2)

Prior to C# 5, you need to re-declare a variable inside the foreach - otherwise it is shared, and all your handlers will use the last string:

foreach (string list in lists)
{
    string tmp = list;
    Button btn = new Button();
    btn.Click += new EventHandler(delegate { MessageBox.Show(tmp); });
}

Significantly, note that from C# 5 onwards, this has changed, and specifically in the case of foreach, you do not need to do this any more: the code in the question would work as expected.

To show this not working without this change, consider the following:

string[] names = { "Fred", "Barney", "Betty", "Wilma" };
using (Form form = new Form())
{
    foreach (string name in names)
    {
        Button btn = new Button();
        btn.Text = name;
        btn.Click += delegate
        {
            MessageBox.Show(form, name);
        };
        btn.Dock = DockStyle.Top;
        form.Controls.Add(btn);
    }
    Application.Run(form);
}

Run the above prior to C# 5, and although each button shows a different name, clicking the buttons shows "Wilma" four times.

This is because the language spec (ECMA 334 v4, 15.8.4) (before C# 5) defines:

foreach (V v in x) embedded-statement is then expanded to:

{
    E e = ((C)(x)).GetEnumerator();
    try {
        V v;
         while (e.MoveNext()) {
            v = (V)(T)e.Current;
             embedded-statement
        }
    }
    finally {
        … // Dispose e
    }
}

Note that the variable v (which is your list) is declared outside of the loop. So by the rules of captured variables, all iterations of the list will share the captured variable holder.

From C# 5 onwards, this is changed: the iteration variable (v) is scoped inside the loop. I don't have a specification reference, but it basically becomes:

{
    E e = ((C)(x)).GetEnumerator();
    try {
        while (e.MoveNext()) {
            V v = (V)(T)e.Current;
            embedded-statement
        }
    }
    finally {
        … // Dispose e
    }
}

---

Re unsubscribing; if you actively want to unsubscribe an anonymous handler, the trick is to capture the handler itself:

EventHandler foo = delegate {...code...};
obj.SomeEvent += foo;
...
obj.SomeEvent -= foo;

Likewise, if you want a once-only event-handler (such as Load etc):

EventHandler bar = null; // necessary for "definite assignment"
bar = delegate {
  // ... code
  obj.SomeEvent -= bar;
};
obj.SomeEvent += bar;

This is now self-unsubscribing ;-p

ReSharper Warning - Access to Modified Closure

The reason for the warning is that inside a loop you might be accessing a variable that is changing. However, the "fix" isn't really doing anything for you in this non-loop context.

Imagine that you had a FOR loop and the if was inside it and the string declaration was outside it. In that case the error would be correctly identifying the problem of grabbing a reference to something unstable.

An example of what you don't want:

string acctStatus

foreach(...)
{
  acctStatus = account.AccountStatus[...].ToString();
  if (!SettableStatuses().Any(status => status == acctStatus))
      acctStatus = ACCOUNTSTATUS.Pending.ToString();
}

The problem is that the closure will grab a reference to acctStatus, but each loop iteration will change that value. In that case it would be better:

foreach(...)
{
  string acctStatus = account.AccountStatus[...].ToString();
  if (!SettableStatuses().Any(status => status == acctStatus))
      acctStatus = ACCOUNTSTATUS.Pending.ToString();
}

As the variable's context is the loop, a new instance will be created each time because we have moved the variable inside the local context (the for loop).

The recommendation sounds like a bug in Resharper's parsing of that code. However, in many cases this is a valid concern (such as the first example, where the reference is changing despite its capture in a closure).

My rule of thumb is, when in doubt make a local.

Here is a real world example I was bitten by:

        menu.MenuItems.Clear();
        HistoryItem[] crumbs = policyTree.Crumbs.GetCrumbs(nodeType);

        for (int i = crumbs.Length - 1; i > -1; i--) //Run through items backwards.
        {
            HistoryItem crumb = crumbs[i];
            NodeType type = nodeType; //Local to capture type.
            MenuItem menuItem = new MenuItem(crumb.MenuText);
            menuItem.Click += (s, e) => NavigateToRecord(crumb.ItemGuid, type);
            menu.MenuItems.Add(menuItem);
        }

Note that I capture the NodeType type local, note nodeType, and HistoryItem crumb.ItemGuid, not crumbs[i].ItemGuid. This ensures that my closure will not have references to items that will change.

Prior to using the locals, the events would trigger with the current values, not the captured values I expected.

Is an Implemenation Depending on Access to Modified Closure Undesirable?

Here is a quick alternative that avoids some of the issues you may be concerned with. Also, as @Servy mentioned just calling a sperate async function will do. The ConcurrentStack just makes it easy to add and clear, additionally more information could be logged than just the count.

public class FaultCounter {

    private ConcurrentStack<Exception> faultsSinceLastSuccess;        

    public async void RunServiceCommand() {
        faultsSinceLastSuccess = new ConcurrentStack<Exception>();
        var faultCounter = StartFaultLogging(new CancellationTokenSource());
        var worker = DoWork(new CancellationTokenSource());
        await Task.WhenAll(faultCounter, worker);
        Console.WriteLine("Done.");
    }

    public async Task StartFaultLogging(CancellationTokenSource cts) {
        while (true && !cts.IsCancellationRequested) {
            Logger.Error($"{faultsSinceLastSuccess.Count} failed attempts to get work since the last known success.");
            faultsSinceLastSuccess.Clear();
            await Task.Delay(300 * 1000);
        }
    }

    public async Task<object> DoWork(CancellationTokenSource cts) {            
        while (true) {
            object work = null;
            try {
                work = await GetWork();
                cts.Cancel();
                return work;
            }
            catch (Exception ex) {
                faultsSinceLastSuccess.Push(ex);
            }
        }
    }
} 

Access to modified closure... but why?

The problem is in the following statement

Because primaryApps is declared within the context of the for loop, primary isn't going to change while I'm processing primaryApps.

There is simply no way for Resharper to 100% verify this. The lambda which references the closure here is passed to function outside the context of this loop: The AppPrimaries.Select method. This function could itself store the resulting delegate expression, execute it later and run straight into the capture of the iteration variable issue.

Properly detecting whether or not this is possible is quite an undertaking and frankly not worth the effort. Instead ReSharper is taking the safe route and warning about the potentially dangerous capture of the iteration variable.

---

Related Topics