0

When reading a post, Starting Tasks In foreach Loop Uses Value of Last Item), the marked answer makes a lot of sense. The author created a new variable, pathCopy, to use in the task. My question is, is this only necessary when Task.ContinueWith() is used?

Here is an example:

    private void GetAuditFiles()
    {
        _auditFiles = new ConcurrentBag<AuditFile>();

        var tasks = new List<Task>();
        foreach (var auditFile in Directory.GetFiles(_properties.AuditFileOutputPath))
        {
            var taskfile = auditFile;

            tasks.Add(Task.Factory.StartNew(() =>
            {
                var file = DeserializeProcessProperties<AuditFile>(File.ReadAllText(taskfile));
                file.filename = Path.GetFileName(taskfile);
                _auditFiles.Add(file);
            }));
        }

        Task.WaitAll(tasks.ToArray());
    }

Do I need to set a variable like this "var taskfile = auditFile;"?

Note: I am using an updated version of VS 2017 and its C# compiler.

Sam Jazz
  • 131
  • 7
  • It's got nothing to do with `ContinueWith`. It does have something to do with the C# 4 compiler and earlier. Are you really using such a compiler in 2018? – Damien_The_Unbeliever Nov 14 '18 at 15:47
  • It's necessary whenever you're changing a variable after using it in a closure and you don't want the closure to observe that change. Whether you're doing that when not using `ContinueWith` is not something we can really say without knowing how you *are* accomplishing that. – Servy Nov 14 '18 at 15:47
  • @Damien_The_Unbeliever There are plenty of situations where you end up closing over a variable that changes in any version of C# from 2.0 to present. Now what can put you into that situation can be slightly different in different versions of the language, but that by no means makes this a C# 2-4 issue exclusively. – Servy Nov 14 '18 at 15:50
  • 2
    I would also caution that the `ContinueWith` shown in the answer (but just copied from the question) is broken code and shouldn't be copied. Each `ContinueWith` is independently executed with no synchronization and they're all updating a shared variable. You may lose a store which changes it from `true` to `false` due to races. – Damien_The_Unbeliever Nov 14 '18 at 15:51
  • @Servy - but for *that* question and answer it very much is the `foreach` behaviour of older versions that the answer was written to address. – Damien_The_Unbeliever Nov 14 '18 at 15:52
  • @Damien_The_Unbeliever We have no idea what actual code this user is attempting to apply that approach to, and whether or not that code involves mutating a variable after closing over it, because we have no idea what their code looks like at all. – Servy Nov 14 '18 at 15:55
  • That question and answer had nothing to do with *Tasks*, much less `ContinueWith`. The actual problem was variable capture by the lambda. The entire code wouldn't be needed in any supported .NET version nowadays - PLINQ could be used to perform all operations in parallel and collect the results, eg with a simple `Min()`: `paths.AsParallel().Select(ProcessPicture).Min();`. If `ProcessPicture` was rewritten as a real async method, it could be `var results=await Task.WhenAll(paths.Select(ProcessPictureAsync));` followed eg by a `var allTrue=results.All(r=>r);` – Panagiotis Kanavos Nov 14 '18 at 15:58
  • @PanagiotisKanavos PLINQ was added in the same version as `Task` was. It's not any newer. – Servy Nov 14 '18 at 16:02
  • @Servy yes, but I was trying to think of ways of rewriting the entire operation at the time. Jon Skeet answered about the closure, not `ContinueWith` anyway. – Panagiotis Kanavos Nov 14 '18 at 16:06
  • @PanagiotisKanavos You implied that your solution is using newer tools. I'm just saying that the solution you provided was available at the time that answer was posted. – Servy Nov 14 '18 at 16:19
  • @Servy - well, we now have the code, and it's entirely about `foreach`. If you hadn't wanted to discuss the only code available previously, you should surely have voted to close but didn't do so. – Damien_The_Unbeliever Nov 14 '18 at 18:02
  • 1
    To OP - you should surely have read the comments and the links to Eric Lippert's Blog included in the answer that explained why this specific issue was no longer relevant – Damien_The_Unbeliever Nov 14 '18 at 18:11

1 Answers1

0

Ok, thanks to damien-the-unbeliever, for pointing me back to Eric Lippert's Blog: closing over the loop variable part two.

So the short answer is yes, if I am c# v 4.0 or earlier. It is unneeded in any case if at c# 5.0 or later.

Sam Jazz
  • 131
  • 7