List index out of range error while creating threads in for loop

Question

I am not too familiar with C# and I need to implement multithreading by creating 5 threads. However, I keep getting an error:

Run-time exception (line 20): Index was out of range. Must be non-negative and less than the size of the collection. Parameter name: index

I am not changing the list at all during program execution, so I'm not sure why I keep getting this error. Here's my code:

using System;
using System.Threading;
using System.Collections.Generic;

namespace Parallelization{
    public class Update{
        //creating a lock object of reference type
        private Object LOCK = new Object(); 
        
        public void StartGroup(){
            Console.WriteLine("Restoring groups");
            List<string> Groups = GetGroupsName();
        
            //parallelizing vm groups
            Thread[] threads = new Thread[Groups.Count];
                
            //create a new thread for each vm group
            for(int i = 0; i < Groups.Count; i++){
                threads[i] = new Thread(() => StartParallel(Groups[i]));
                Console.WriteLine("Starting thread {0}", I);              //ERROR
            }
            for(int i = 0; i < Groups.Count; i++){
                threads[i].Start();
            
            }
        }
        
        //callback for each thread
        private void StartParallel(string GroupName){
            try{
                //Restore group
                lock(LOCK){
                    RestoreGroup(GroupName); 
                }
                
            }
            catch(Exception ex){
                Console.WriteLine("Exception encountered! restoration failed");
            }       
        }   
        
        //function to restore each vm group
        private void RestoreGroup(string GroupName){
            Console.WriteLine(GroupName + " restored!");
        }
            
        //function to obtain a list of group names
        private List<string> GetGroupsName(){
            List<string> groups = new List<string>
                {"group1","group2","group3","group4","group5"};
            return groups;
        }   
    }
    
    public class Groups{
        //calling StartGroup
        public static void Main(string[] args){
            Update update = new Update();
            update.StartGroup();
                
        }
    }
}

Also manually creating `Thread`s usually is considered a bad practice in C#. Usually you should prefer `Task`'s. — Guru Stron, Jul 12 '21 at 01:07
Has that duplicate made it clear to you why your code is failing? — Enigmativity, Jul 12 '21 at 01:10
@GuruStron - To be clear, using tasks alone does not eliminate the issue the OP is facing. — Enigmativity, Jul 12 '21 at 01:11
thanks for all the suggestions. My code works now. @GuruStron, I will look into tasks, thanks for the suggestion — pesky_programmer, Jul 12 '21 at 01:16
@pesky_programmer - What's the point of starting the threads and then immediately `lock`ing them all? — Enigmativity, Jul 12 '21 at 01:59
@Enigmativity I'm using a lock to avoid a race condition when all the threads try to access the restore function at the same time. I suppose what you mean to say is I should be using a time delay? I need to make these operations run in parallel and I can't think of a better way to do it — pesky_programmer, Jul 12 '21 at 08:33
@pesky_programmer - My point is that the `lock` forces them to run in series and not parallel. There's no point using threads and then locking. — Enigmativity, Jul 12 '21 at 08:46
@Enigmativity hmm, I get what you're saying but isn't that always the case with locks? I mean whenever multiple threads try to access the same function/variable, there's no other way than granting access to it sequentially? — pesky_programmer, Jul 12 '21 at 08:48
@pesky_programmer - Yes, that's my point. Why are you locking? — Enigmativity, Jul 12 '21 at 08:51
@Enigmativity because all the threads need to access it. Won't it lead to a race condition? — pesky_programmer, Jul 12 '21 at 08:53
@Enigmativity or do you mean to say, locks are not required since the function is not being modified by any of the threads? — pesky_programmer, Jul 12 '21 at 08:54
@pesky_programmer - There's nothing in your code that would cause a race condition. You need to show us your real code if this isn't it. — Enigmativity, Jul 12 '21 at 08:54
@Enigmativity not sure if I have permission to share it. My task is to make the restoration of each group parallel. So, I used multi-threading, but you're probably right about there being no race condition in the code. I don't think the restore function is modifying any shared resource either — pesky_programmer, Jul 12 '21 at 08:59
@pesky_programmer - In the question you've asked there is no benefit. In your real code there may be, but if you're launching a thread and then locking in your real code there is no benefit, just lots of overhead. — Enigmativity, Jul 12 '21 at 09:04
@Enigmativity if in my real code, there is a race condition, so I would need to use a lock and there would be no way to avoid the overhead. Plus, it would make the access to the restore function by different threads sequential. Am I right in my assumptions? — pesky_programmer, Jul 12 '21 at 09:09
I'd have to see your real code. There's often a smarter way to do things, but I can't tell until I see the code. If you can't show the code then I would have to conclude that not using threads is faster. — Enigmativity, Jul 12 '21 at 09:11
I see, thanks for the suggestion, I'll try to think of another, faster way to implement my code @Enigmativity — pesky_programmer, Jul 12 '21 at 09:25
@Enigmativity that's why the duplicate I've added has nothing to do with `Task`'s =) — Guru Stron, Jul 12 '21 at 18:20
@Enigmativity, I'm afraid not, I don't have the permission to do it. thanks for all you help though:) — pesky_programmer, Jul 12 '21 at 20:23

List index out of range error while creating threads in for loop

0 Answers0