Click here to Skip to main content
15,867,488 members
Please Sign up or sign in to vote.
5.00/5 (1 vote)
See more:
I put together a multi-threaded application and seem to have made a mistake... quite fascinating and all, but not good.
I was hoping someone had a suggestion or you may say (like I thought "that should not be a problem")
I'll try to make this as simple and short as possible, but still show the situation. This is written in NotePad++, so if there are syntax errors, they are not the issue. The issue is a thread working on each class in the generic List.
Consider:

public class main()
{
    c_Phones_Info cPhones_Info = new c_Phones_Info();
	cPhones_Info.start();
}

public class c_PhoneInfo
{
   public string PhoneNumber {get; set;}
   public string Name {get; set;}
   public c_PhoneInfo (string sPhoneNumber, string sName)
   { PhoneNumber = sPhoneNumber; Name = sName; }
   public void getNameFromDatabase()
   {
        DataSet ds = new DataSet();
        RealLongRunningSQLProcessToGetName(this.PhoneNumber);
		...
		this.Name = ds.Tables[0].Rows[0]["name"].ToString()
   }
}

public class c_Phones_Info
{
    public List<cPhoneInfo> lstcPhoneInfo;
	public c_Phones_Info(){ lstcPhoneInfo = new List<c_PhoneInfo>()}
	
	public void start()
	{
	    int iMaxPhonesToProcessAtATime = 10;
	    int iNumberOfPhonesBeingProcessed = 0;
	   
	    while (true)
	    {		 
		    iNumberOfPhonesBeingProcessed = 0; 
	        for (int iCounter = 0; iCounter < lstcPhoneInfo.Count; iCounter++)
		        if (lstcPhoneInfo[iCounter].name == "NADA")
		            iNumberOfPhonesBeingProcessed++;		
		
		    if(iNumberOfPhonesBeingProcessed < iMaxPhonesToProcessAtATime)
			{   
			    List<cPhoneInfo> LocallstcPhoneInfo = new List<cPhoneInfo>();
			    getPhoneNumbersFromDatabase(LocallstcPhoneInfo);
			    foreach (c_PhoneInfo cPhoneInfo in LocallstcPhoneInfo)
                   lstcPhoneInfo.Add(new c_PhoneInfo (cPhoneInfo.PhoneNumber, "NADA"));

			    foreach (c_PhoneInfo cPhoneInfo in lstcPhoneInfo)
				{
                    if(cPhoneInfo.Name  == "NADA")
				    {				   
				        Task task1 = new Task(new Action(cPhoneInfo.getNameFromDatabase()));
                        task1.Start();
				    }
				}
 	        }  
			Thread.Sleep(2000);
	    } 
	}
    public static void getPhoneNumbersFromDatabase(List<cPhoneInfo> LocallstcPhoneInfo)
    {
        DataSet ds = new DataSet()
        ...
		for (int ii = 0; ii < ds.Tables[0].Rows.Count; ii++)
		    LocallstcPhoneInfo.Add(new c_PhoneInfo (
                this.PhoneNumber = ds.Tables[0].Rows[ii]["PhoneNumber"].ToString();
                this.Name = "NADA" // whatever
		    	);
    }		
}


Hopefully this is enough to make it clear. The application is more complicated, but I think this is the problem.
At no time do two threads try to write to the same class instance. Reads are unrestricted.
They do though quite potentially write to two different classes in the same List of classes at the same time. I think that is the problem.
Actually, in the application I remove the classes from the list (yes, backwards) when the processing is complete.
"Exception": "Collection was modified; enumeration operation may not execute."
It makes sense in a way (in hindsight) so I suspect there is a fairly standard solution to the problem. It does not seem practical to lock the List. Each class is heavily processed... Though I am open to suggestion.
Do I have to make say 10 classes up front (not in a list) and recycle them through threads or maybe even make the threads part of the class?
Any suggestions would be appreciated.
Thanks, Mike
Posted
Updated 21-Jan-15 8:59am
v2
Comments
Sergey Alexandrovich Kryukov 21-Jan-15 15:45pm    
Despite of apparent problems you created, this is an interesting, practically important question; I voted 5 for it.
Sorry that I did not bother to find your bug. The nature of it is quite clear. Instead, I think it would be more useful if you consider some of the design principles I suggested (again, your ideal of using lock-less approach is very good, no matter what is your application of it) and review your code critically, most likely create it again from scratch, with better understanding.

Such parts of code take critical responsibility and not fixable by just testing (should be well worked through theoretically instead), so your time won't be wasted at all.

—SA
Michael Breeden 22-Jan-15 12:19pm    
Sir, thank you for your expertise. I am constructing a test application from the above code (LOL) and will test it with different scenarios including 1. The thread wrapper you pointed me to, 2. A concurrent collection (I expect a performance problem) and 3. Not putting the classes in a collection, responding to an event on thread completion. I will post the result

1 solution

Yes, this exception suggests that you really miss the lock. I mean, yes, your understanding that lock is impractical is basically valid (they say: "best synchronization is no synchronization"). The problem is: you still do the operation which needs locking, so the solution will be: either 1) add lock on the same lock object to the statement; 2) or avoid such operation. Another approach, which you will eventually need anyway, with this or another problem, is combined: use a lock but minimize its use.

I hope you understand how to use the lock, so I want to set aside this trivial issue. Your exception suggests that the failure to lock was manifested when you tried to modify the collection. What does it mean? First, let's consider the simple model without locking at all. Let's say, you have one "main" thread (I say nothing about its nature; it does not have to be the UI thread or the thread of your application entry point, it can be anything) used to manipulate collection, and N thread, each corresponding the a collection element, to manipulate elements.

What would it mean, "manipulate collection" and "manipulate elements"? First, the elements. Each thread corresponding to an element, can modify the state of this element's object and never anything else. Importantly, this object is of the reference type (you said, "classes", so everything is fine), and this element's thread should not modify the referential identity of the element object. Naturally, such thread should not add/remove and even access elements of collection, they should not even traverse the elements, inquire the collection length.

And the main thread should add/remove elements, and do everything with the collection, but never access element objects, not even read them. The element's thread can be started only when the element is created, and the element can be removed, without stopping the thread. That is, adding and removing elements can be done when elements' threads are executing; this is guaranteed by the rule of not touching referential identities. For collaboration, lock is still required. For example, the main thread can iterate the elements. But what to do with each one? As soon as elements members need to get involved, an interlock with the element's thread is required.

One modification of these scheme could be using upgradeable lock System.Threading.ReaderWriterLockSlim:
https://msdn.microsoft.com/en-us/library/system.threading.readerwriterlockslim%28v=vs.110%29.aspx[^].

It's use goes beyond the lock-less schema, so please learn the performance implications yourself. This are understandable but not so trivial.

And yes, you probably can simplify your code while preserving thread safety. One of the approaches I use all the time is the concept of thread wrapper I used well before .NET (I am the author of one of the earliest implementations of threads, when they weren't even introduced in C++ yet). I presented this idea in my past answers:
How to pass ref parameter to the thread[^],
Change parameters of thread (producer) after it is started[^] (this is how to encapsulate and hide locking, by the way),
http://www.codeproject.com/Answers/485734/MultiThreadingplusinplusC-23#answer4[^],
Making Code Thread Safe[^].

Main point of the wrapper is the access of the members from a thread and isolating and encapsulation of this access. The idea is using an instance (non-static) thread method and getting access through "this", while keeping control over the access.

—SA
 
Share this answer
 
v3

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900