Threading: incorrect Variable passing C#

Question

users. I'm running into a problem that i can't find an answer for. I'm kind of new to Threading (in C#), and ran into this problem. I have this image editor with effects, but since it ran too slow, i tried to split it up in threads. The problem is that he always runs the "CreatePreview" command with the last item in the list of effects. So if i activated effects: "Black/White","Sature" and "GreenFilter", it will try to create 3 previews with a greenfilter.

Could anyone help me out with this problem?

private void CreatePreviews(string fileName, List<IEffect> effects)
{
    List<Task> tasks = new List<Task>();
    foreach (var effect in effects)
    {
        //previews.Add(effect, CreatePreview(fileName, effect));
        Task task = new Task(delegate()
        {
            string result = CreatePreview(fileName, effect);
            Dispatcher.BeginInvoke(new Action(
            delegate()
            {
                ShowPreview(result, effect.DisplayName);
            }));

        });
        task.Start();
    }
}

You may need to provide the code for `CreatePreview` and `ShowPreview` as well so we can determine if they are properly thread-safe. For instance, if you use a global variable inside `CreatePreview` for the "current effect", all three will reference the same effect, which is the one set by the last task. — mellamokb, May 21 '12 at 23:22
Search for 'access to modified closure' or 'captured variables' and you'll see your problem., for example: http://stackoverflow.com/questions/235455/access-to-modified-closure — RJ Lohan, May 21 '12 at 23:24
Duplicate of [Closing over the loop variable in C#.](http://stackoverflow.com/questions/9626051/closing-over-the-loop-variable-in-c-sharp) — Chris Hannon, May 21 '12 at 23:26
Closing this as your example no longer illustrates your problem. In the future, leave the 'wrong' code in the question so it provides context to the answers that fix it :) — Tim Post, Jun 12 '12 at 03:54

score 5 · Answer 1 · edited Apr 02 '14 at 11:55

I can't test right now, but I'm pretty sure that your problem is that you're closing over the loop variable.

Take a copy of your loop variable and close over that instead:

foreach (var effect in effects)
{
    var effectCopy = effect;

    //previews.Add(effectCopy, CreatePreview(fileName, effectCopy));
    Task task = new Task(delegate()
    {
        string result = CreatePreview(fileName, effectCopy);
        Dispatcher.BeginInvoke(new Action(delegate()
        {
            ShowPreview(result, effectCopy.DisplayName);
        }));
    });

    task.Start();
}

(Or wait for C#5, which automatically closes over a fresh copy of the variable on each iteration.)

Thank you! This worked. I'm gonna dig into the whole closure thing just a bit more now. — user1408984, May 22 '12 at 09:17

SimpleVar · Answer 2 · 2012-05-22T17:23:59.710

You must save the current effect into a variable within the loop, to prevent access to modified closure in the delegate, which means that all delegates access the loop-variable, which in the end, has the value of the last element you loop on, and therefore all Tasks run with the last effect. To prevent that:

private void CreatePreviews(string fileName, List<IEffect> effects)
{
    List<Task> tasks = new List<Task>();

    foreach (var effect in effects)
    {
        var mcEffect = effect;

        Task task = new Task(delegate()
            {
                string result = CreatePreview(fileName, mcEffect);
                Dispatcher.BeginInvoke(new Action(
                delegate()
                {
                    ShowPreview(result, effect.DisplayName);
                }));
            });

        task.Start();
    }
}

I like giving the prefix mc to note modified closure.

score 1 · Answer 3 · answered May 21 '12 at 23:29

Your delegate needs to create a local copy of the value of effect so that the value when it is actually evaluated doesn't change due to the loop iterator queuing all of the changes before the threads actually evaluate effect.

foreach(var effect in effects)
{
    var localEffect = effect;
    var task = new Task(()=>
        {
            var result = CreatePreview(fileName, localEffect);
            Dispatcher.BeginInvoke(()=> ShowPreview(result, localEffect.DisplayName));
        });
    task.Start();
}

This will force the individual threads to properly close on the creation-time value of effect. This is due to the way anonymous delegates create hidden classes in the background.

See this article on why what you have created didn't quite create a lexical closure, but by copying effect to localEffect it will... Anonymous method article.

score 0 · Answer 4 · answered May 21 '12 at 23:28

0

Multi-threading is quite a complex matter, with lots of potential for problems.

Definitely read an article or two on Task Parallel Library (or a book).

A potentially more correct version using TPL will look something like:

Parallel.ForEach(effects, currentEffect =>
{
    string result = CreatePreview(fileName, currentEffect );
    ShowPreview(result, effect.DisplayName);
}

PS. Keep in mind that the best practice in this case is to actually parallelize each filtering operation (or even better to offload it to the GPU).

answered May 21 '12 at 23:28

Boris Yankov

1,530
14
21

This doesn't address the root cause, which is closing over a loop variable. – Odrade May 21 '12 at 23:34
And will this fix the fact he is creating too many threads? And the fact the code is naive possibly wrong in numerous of ways? – Boris Yankov May 22 '12 at 00:13
And by the way, yes it completely fixes the original problem, still thanks for the downvote. – Boris Yankov May 22 '12 at 00:14
1

@Odrade: Well, yes, actually it does fix the closure problem. – Brian Gideon May 22 '12 at 01:29
The downside to this is that `ShowPreview` is not being called on the UI thread. – Brian Gideon May 22 '12 at 01:30
@Boris, Brian: I should rephrase: while the code in this answer resolves the problem, it does not explain the root cause which is closing over the loop variable. While I agree there are other problems in the original code than this, they are not the subject of the question. Try to think from the OP's perspective; does your answer actually explain anything to them? – Odrade May 22 '12 at 23:13
@Orade, yes I believe it does. It points to TPL which is a more high level approach to this. I didn't go in more concrete details, but his code has many problems besides the closure. Like too many potential threads, exception handling, UI affinity etc. etc. etc. All of these are not trivial tasks and are resolved much more easily with TPL. – Boris Yankov May 23 '12 at 13:20
@Boris You never mentioned what caused the original problem the OP was having. This is an important piece of information that the OP needs to learn. If someone asks you "Why is X broken" and you answer "Use Y instead" then you haven't answered the question. The suggestion may be helpful, but the question still stands. – Odrade Jun 11 '12 at 22:44

Threading: incorrect Variable passing C#

4 Answers4