CODEWITHSUNDEEP
c#multithreadinglistlockingsynchronization
gStation
gStation

Reputation: 83

My list is not thread safe even though I lock it

I am getting:

"collection was modified enumeration operation may not execute" at this lline:

foreach (Message msg in queue)

after a while.

I have to use .NET 2.0.

The two operations I make on the private List<> named "queue" are as follows:

// 1st function calls this
lock (queue)
{
      queue.Add(msg);
}

// 2nd function calls this
lock (queue)
{
      using (StreamWriter outfile = new StreamWriter("path", true)
      {
             foreach (Message msg in queue) // This is were I get the exception after a while)
             {
                  outfile.WriteLine(JsonConvert.SerializeObject(msg)); 
             }

              queue = new List<Message>();
      }
}

What am I doing wrong?

Upvotes: 2

Views: 345

Answers (2)

Marc Gravell
Marc Gravell

Reputation: 1062770

(below; well, I can't come up with a race condition that would cause this... but: who knows...)

Firstly, you really need to look for some other code that talks to the list; the problem is not in the code you post. How do I know this? Because at the point you are enumerating (foreach (Message msg in queue)), you have a lock on queue, and we haven't done anything with the (very dodgy, but unrelated) re-assignment of the lock object.

For this foreach to error means that something else is mutating the list. First thing to do, very simply, is to rename the list field. This will very quickly show you if some other code touches the list. Also check that you never expose the list outside this code, i.e. never return queue; from anywhere.

The problem does not appear to be in the code you show. Reassigning the lock object is bad practice and you shouldn't do that - but: I can't see a scenario (with the code shown) where it would actually break it.

Lists are not the best model here, and reassigning a lock object is not a good idea. If only there was an inbuilt type designed to represent a queue...

private readonly Queue<Message> queue = new Queue<Message>();
...
lock (queue) {
    queue.Enqueue(msg);
}

// 2nd function calls this
lock (queue) {
    if(queue.Count == 0) continue; // taken from comments

    using (StreamWriter outfile = new StreamWriter("path", true) {
        while(queue.Count != 0) {
            Message msg = queue.Dequeue();
            outfile.WriteLine(JsonConvert.SerializeObject(msg)); 
        }
    }
}

no need to clear, since Dequeue does that intrinsically and efficiently.

Upvotes: 3

Md Kamruzzaman Sarker
Md Kamruzzaman Sarker

Reputation: 2407

the parameter which is used by the lick statement should be readonly. See this link

Use readonly private object instead of queqe

Code should be

eadonly object _object = new object();
// 1st function calls this
lock (_object)
{
      queue.Add(msg);
}

// 2nd function calls this
lock (_object)
{
      using (StreamWriter outfile = new StreamWriter("path", true)
      {
             foreach (Message msg in queue) // This is were I get the exception after a while)
             {
                  outfile.WriteLine(JsonConvert.SerializeObject(msg)); 
             }

              queue = new List<Message>();
      }
}

Upvotes: 1

Related Questions