0

Update 19.June.2020: Alexei Levenkov suggested Captured variable in a loop in C# - my issue may have nothing to do with threading. I've been using C# for almost 20 years and never knew this and have never knowingly been affected by it. Now I'm a little worried about everything that I've ever done...Thanks Alexei!

I have some C# code that creates threads to pass parameters to a method to page through some data. One of those parameters to the method is the counter for the loop that creates the threads.

Using Console.WriteLine(), I can see that the value to pass to the method is 0 on the first iteration, but the method receives 100, which is the value of the increment for the loop, as if it's receiving the value after the subsequent iteration of the loop or the iterations are sharing a framestack or whatever somehow. It seems impossible because skup is just a value type int.

I do not have this problem when Repository.Configuration.UseThreading is false.

I am not very familiar with threading, so I wonder what I have told it to do that could cause this. I tried creating an Int32 from skip just to pass to the method but had the same result. Whatever the issue is, it's hard for me to understand the consequences where I pass other parameters.

The first thing the method in the thread does is show the value that it recives, which is 100:

private void LoadBatch(EntryPointer entryPointer, ContentTypeWrapper contentTypeWrapper,
    int skip, Action<EntryPointer> action)
{
    Console.WriteLine("LoadBatch: " + skip);
    //...

Here is the code that creates the threads:

foreach (ContentTypeWrapper contentTypeWrapper in contentTypeWrappers)
{
    if (contentTypeWrapper.EntriesLoaded == contentTypeWrapper.EntryCount)
    {
        Repository.Instrument.Warn(this, $"contentTypeWrapper.ContentTypeUid"
            + $" already loaded or {contentTypeWrapper.EntryCount} entries; skip");
        continue;
    }

    for (int skip = 0; skip < contentTypeWrapper.EntryCount;
        skip += Repository.Configuration.QueryLimit)
    {
        if (Repository.Configuration.UseThreading)
        {
            new Thread(() => LoadBatch(entryPointer, contentTypeWrapper, skip, action))
                .Start();
        }
        else
        {
            LoadBatch(entryPointer, contentTypeWrapper, skip, action);
        }
    }

If I make this change, it works the way that I would expect:

for (int skip = 0; skip < contentTypeWrapper.EntryCount;
    skip += Repository.Configuration.QueryLimit)
{
    int mySkip = skip;

    if (Repository.Configuration.UseThreading)
    {
        Console.WriteLine("Passing to LoadBatch: " + mySkip);
        new Thread(() => LoadBatch(entryPointer, contentTypeWrapper, mySkip, action))
           .Start();
Theodor Zoulias
  • 24,585
  • 5
  • 40
  • 69
Deliverystack
  • 1
  • 3
  • When you start using lambdas, `for` loops become dangerous. `foreach` [is safe](https://stackoverflow.com/questions/12112881/has-foreachs-use-of-variables-been-changed-in-c-sharp-5), unless you compile with a C# version earlier than 5.0. – Theodor Zoulias Jun 20 '20 at 10:04
  • Thank you so much for bringing this to my attention. I am an old-school programmer, just learning about new concepts like lambdas and threading. This one really surprises me - I think it was a flaw in the language design. – Deliverystack Jun 20 '20 at 23:56
  • Yeah, surely the compiler could add this internal variable `int mySkip = skip` on behalf of the developer, but this could have unexpected side-effects. For example if your code inside the loop had a `skip++`, what should the compiler do? Increment the `skip`, the `mySkip`, or both? I think that the `for` loop is just too unrestrained, contrary to the disciplined `foreach`, so the compiler can't fix the programmer's errors in a safe and predictable way. It could emit some warnings though. – Theodor Zoulias Jun 21 '20 at 00:11
  • Maybe the problem started when the C# designers decided to make the closures mutable. Here is what a closure looks like: [Closure captured variable modifies the original as well](https://stackoverflow.com/questions/15275637/closure-captured-variable-modifies-the-original-as-well). And here is a discussion about this decision: [Are there any good reasons why closures aren't immutable in C#?](https://stackoverflow.com/questions/483944/are-there-any-good-reasons-why-closures-arent-immutable-in-c) – Theodor Zoulias Jun 21 '20 at 00:30
  • One more link with links to additional resources. Interesting topic that has created a great deal of confusion. https://stackoverflow.com/questions/271440/captured-variable-in-a-loop-in-c-sharp @Theodor I guess I would treat that skip++ as an illegal change to the iterator or a change to the variable in the nested scope, possibly throw a compiler warning. I wonder what search terms we could include that would make it easier for developers with related issues to find these posts. Not sure StackOverflow should have closed this thread - the moderators are getting worse. – Deliverystack Jun 21 '20 at 16:18
  • What would be the optimal way to recode this to avoid the issue? Is a nested variable like mySkip really the best solution? for (int skip = 0; skip < contentTypeWrapper.EntryCount; skip += Repository.Configuration.QueryLimit) – Deliverystack Jun 21 '20 at 16:28
  • I think that a local variable is the easiest thing to do in this case. If you would like to do it more enterprisey, you could consider using `foreach` and a `Partitioner`: `foreach (var range in Partitioner.Create(0, contentTypeWrapper.EntryCount, Repository.Configuration.QueryLimit).GetDynamicPartitions()) { new Thread(() => LoadBatch(entryPointer, contentTypeWrapper, range.Item1, action)).Start(); }` Btw this site is run mainly by users, not moderators. Any member with 3000 [reputation](https://stackoverflow.com/help/privileges) or more can cast close and reopen votes. – Theodor Zoulias Jun 21 '20 at 18:37
  • Another question is whether the issue with trapped variables in for/foreach loops affects Parallel.ForEach and Parallel.For. I think not. – Deliverystack Jul 05 '20 at 20:54
  • Regarding the `Parallel.ForEach` and `Parallel.For`, the supplied lambda is invoked once per item/index, and the item/index is passed as argument, so it is local to the lambda, and not a captured variable from the outer scope. So there is no problem with the item/index. You can still run into trouble if you try to mutate the collection itself from inside the lambda, unless the collection is thread-safe and you know what you are doing. The safest way to do parallel processing is to use PLINQ, which is purely functional and has no side-effects. – Theodor Zoulias Jul 06 '20 at 02:31

0 Answers0