By cdonner


2009-03-03 02:01:58 8 Comments

I can't get to the bottom of this error, because when the debugger is attached, it does not seem to occur. Below is the code.

This is a WCF server in a Windows service. The method NotifySubscribers is called by the service whenever there is a data event (at random intervals, but not very often - about 800 times per day).

When a Windows Forms client subscribes, the subscriber ID is added to the subscribers dictionary, and when the client unsubscribes, it is deleted from the dictionary. The error happens when (or after) a client unsubscribes. It appears that the next time the NotifySubscribers() method is called, the foreach() loop fails with the error in the subject line. The method writes the error into the application log as shown in the code below. When a debugger is attached and a client unsubscribes, the code executes fine.

Do you see a problem with this code? Do I need to make the dictionary thread-safe?

[ServiceBehavior(InstanceContextMode=InstanceContextMode.Single)]
public class SubscriptionServer : ISubscriptionServer
{
    private static IDictionary<Guid, Subscriber> subscribers;

    public SubscriptionServer()
    {            
        subscribers = new Dictionary<Guid, Subscriber>();
    }

    public void NotifySubscribers(DataRecord sr)
    {
        foreach(Subscriber s in subscribers.Values)
        {
            try
            {
                s.Callback.SignalData(sr);
            }
            catch (Exception e)
            {
                DCS.WriteToApplicationLog(e.Message, 
                  System.Diagnostics.EventLogEntryType.Error);

                UnsubscribeEvent(s.ClientId);
            }
        }
    }


    public Guid SubscribeEvent(string clientDescription)
    {
        Subscriber subscriber = new Subscriber();
        subscriber.Callback = OperationContext.Current.
                GetCallbackChannel<IDCSCallback>();

        subscribers.Add(subscriber.ClientId, subscriber);

        return subscriber.ClientId;
    }


    public void UnsubscribeEvent(Guid clientId)
    {
        try
        {
            subscribers.Remove(clientId);
        }
        catch(Exception e)
        {
            System.Diagnostics.Debug.WriteLine("Unsubscribe Error " + 
                    e.Message);
        }
    }
}

13 comments

@open and free 2014-11-11 12:12:12

Why this error?

In general .Net collections do not support being enumerated and modified at the same time. If you try to modify the collection list during enumeration, it raises an exception. So the issue behind this error is, we can not modify the list/dictionary while we are looping through the same.

One of the solutions

If we iterate a dictionary using a list of its keys, in parallel we can modify the dictionary object, as we are iterating through the key-collection and not the dictionary(and iterating its key collection).

Example

//get key collection from dictionary into a list to loop through
List<int> keys = new List<int>(Dictionary.Keys);

// iterating key collection using a simple for-each loop
foreach (int key in keys)
{
  // Now we can perform any modification with values of the dictionary.
  Dictionary[key] = Dictionary[key] - 1;
}

Here is a blog post about this solution.

And for a deep dive in StackOverflow: Why this error occurs?

@Tamb 2018-11-30 21:51:39

Absolutely fantastic explanation.

@ciammarino 2019-02-20 17:40:47

Agreed! Thank you for explaining what was happening and WHY!! This was so helpful.

@Meghraj 2018-12-13 05:33:59

There is one link where it elaborated very well & solution is also given. Try it if you got proper solution please post here so other can understand. Given solution is ok then like the post so other can try these solution.

for you reference original link :- https://bensonxion.wordpress.com/2012/05/07/serializing-an-ienumerable-produces-collection-was-modified-enumeration-operation-may-not-execute/

When we use .Net Serialization classes to serialize an object where its definition contains an Enumerable type, i.e. collection, you will be easily getting InvalidOperationException saying "Collection was modified; enumeration operation may not execute" where your coding is under multi-thread scenarios. The bottom cause is that serialization classes will iterate through collection via enumerator, as such, problem goes to trying to iterate through a collection while modifying it.

First solution, we can simply use lock as a synchronization solution to ensure that the operation to the List object can only be executed from one thread at a time. Obviously, you will get performance penalty that if you want to serialize a collection of that object, then for each of them, the lock will be applied.

Well, .Net 4.0 which makes dealing with multi-threading scenarios handy. for this serializing Collection field problem, I found we can just take benefit from ConcurrentQueue(Check MSDN)class, which is a thread-safe and FIFO collection and makes code lock-free.

Using this class, in its simplicity, the stuff you need to modify for your code are replacing Collection type with it, use Enqueue to add an element to the end of ConcurrentQueue, remove those lock code. Or, if the scenario you are working on do require collection stuff like List, you will need a few more code to adapt ConcurrentQueue into your fields.

BTW, ConcurrentQueue doesnât have a Clear method due to underlying algorithm which doesnât permit atomically clearing of the collection. so you have to do it yourself, the fastest way is to re-create a new empty ConcurrentQueue for a replacement.

@JaredPar 2009-03-03 02:10:17

What's likely happening is that SignalData is indirectly changing the subscribers dictionary under the hood during the loop and leading to that message. You can verify this by changing

foreach(Subscriber s in subscribers.Values)

To

foreach(Subscriber s in subscribers.Values.ToList())

If I'm right, the problem will dissapear

Calling subscribers.Values.ToList() copies the values of subscribers.Values to a separate list at the start of the foreach. Nothing else has access to this list (it doesn't even have a variable name!), so nothing can modify it inside the loop.

@mishal153 2010-05-19 09:25:47

BTW .ToList() is present in System.Core dll which is not compatible with .NET 2.0 applications. So you might need to change your target app to .Net 3.5

@aleafonso 2011-09-19 09:46:25

This is great. I did it with an ArrayList and it also worked perfectly (obviously with ToArray())

@PositiveGuy 2012-02-29 06:13:28

I do not understand why you did a ToList and why that fixes everything

@BlueRaja - Danny Pflughoeft 2012-04-20 12:37:42

@CoffeeAddict: The issue is that subscribers.Values is being modified inside the foreach loop. Calling subscribers.Values.ToList() copies the values of subscribers.Values to a separate list at the start of the foreach. Nothing else has access to this list (it doesn't even have a variable name!), so nothing can modify it inside the loop.

@Johnny_D 2014-04-30 09:37:47

@BlueRaja-DannyPflughoeft so do you mean while ToList is no applied we work with references to same list? Even though we already started looping through new list?

@Sriram Sakthivel 2015-05-15 08:43:33

Note that ToList could also throw if collection was modified while ToList is executing.

@Vishnu Pedireddi 2015-06-09 21:31:40

@Zapnologica You will not a null pointer becuase C# is managed code. The instance will be disposed by the GC only after all the references are released. What you say will be true if it were C++. But, I do agree, it might lead to inconsistencies because you may be working with a stale enumeration.

@Groo 2015-06-15 13:26:47

I am pretty sure think doesn't fix the issue, but merely makes it harder to reproduce. ToList isn't an atomic operation. What's even funnier, ToList bascially does its own foreach internally to copy items into a new list instance, meaning you fixed a foreach problem by adding an additional (although quicker) foreach iteration.

@Jogi 2016-05-19 22:01:42

I'm getting this same error onList.

@KthProg 2017-01-27 17:30:54

I think the concerns about ToList not being atomic are very odd. This is not multi-threaded code. To think that the loop may start before ToList completes is fundamentally misunderstanding how programming languages work. It will not multithread parts of your code at random, or execute the code out of order. Also, InstanceContextMode.Single ensures that it only handles one request at a time.

@Kugan Kumar 2017-06-27 08:14:17

@SriramSakthivel how about using ToArray() ?

@Sriram Sakthivel 2017-06-28 17:28:23

@KuganKumar ToArray isn't any different from ToList. It will also throw exception if modified in another thread while executing the ToArray method.

@Mike Upjohn 2017-09-04 16:28:52

Had this issue while looping through a dictionary of items and adding mid flow, and the above .ToList() method fixed this.

@NickG 2017-10-25 08:27:21

What if you get this problem on a collection which is already a list?

@Ben Call 2018-09-28 16:19:16

For what it's worth... the poster didn't say it would fix the issue... He said it could be used to verify/confirm the issue

@Mark Aven 2018-07-22 14:14:46

Okay so what helped me was iterating backwards. I was trying to remove an entry from a list but iterating upwards and it screwed up the loop because the entry didn't exist anymore:

for (int x = myList.Count - 1; x > -1; x--)
                        {

                            myList.RemoveAt(x);

                        }

@Mike 2015-03-04 20:55:23

I've seen many options for this but to me this one was the best.

ListItemCollection collection = new ListItemCollection();
        foreach (ListItem item in ListBox1.Items)
        {
            if (item.Selected)
                collection.Add(item);
        }

Then simply loop through the collection.

Be aware that a ListItemCollection can contain duplicates. By default there is nothing preventing duplicates being added to the collection. To avoid duplicates you can do this:

ListItemCollection collection = new ListItemCollection();
            foreach (ListItem item in ListBox1.Items)
            {
                if (item.Selected && !collection.Contains(item))
                    collection.Add(item);
            }

@Jamie 2018-03-15 16:37:00

How would this code prevent duplicates from being entered into the database. I have written something similar and when I add new users from the listbox, if I accidentally keep one selected that is already in the list, it will create a duplicate entry. Do you have any suggestions @Mike ?

@Mike 2018-03-16 13:58:02

@jlg See my updated answer above.

@Daniel Moreshet 2014-06-16 07:29:50

I had the same issue, and it was solved when I used a for loop instead of foreach.

// foreach (var item in itemsToBeLast)
for (int i = 0; i < itemsToBeLast.Count; i++)
{
    var matchingItem = itemsToBeLast.FirstOrDefault(item => item.Detach);

   if (matchingItem != null)
   {
      itemsToBeLast.Remove(matchingItem);
      continue;
   }
   allItems.Add(itemsToBeLast[i]);// (attachDetachItem);
}

@Kaspars Ozols 2015-04-21 15:59:36

This code is wrong and will skip some items in the collection if any element will be removed. For example: you have var arr = ["a", "b", "c"] and in first iteration (i = 0) you remove element at position 0 (element "a"). After this all the array elements will move one position up and array will be ["b", "c"]. So, in the next iteration (i=1) you will check element at position 1 which will be "c" not "b". This is wrong. To fix that, you have to move from bottom to the top

@Jogi 2016-05-19 21:58:32

Above code doesn't work with lists.

@Mitch Wheat 2009-03-03 02:13:12

When a subscriber unsubscribes you are changing contents of the collection of Subscribers during enumeration.

There are several ways to fix this, one being changing the for loop to use an explicit .ToList():

public void NotifySubscribers(DataRecord sr)  
{
    foreach(Subscriber s in subscribers.Values.ToList())
    {
                                              ^^^^^^^^^  
        ...

@vivek 2017-03-16 07:20:55

InvalidOperationException- An InvalidOperationException has occurred. It reports a "collection was modified" in a foreach-loop

Use break statement, Once the object is removed.

ex:

ArrayList list = new ArrayList(); 

foreach (var item in list)
{
    if(condition)
    {
        list.remove(item);
        break;
    }
}

@x4000 2009-03-03 06:58:46

A more efficient way, in my opinion, is to have another list that you declare that you put anything that is "to be removed" into. Then after you finish your main loop (without the .ToList()), you do another loop over the "to be removed" list, removing each entry as it happens. So in your class you add:

private List<Guid> toBeRemoved = new List<Guid>();

Then you change it to:

public void NotifySubscribers(DataRecord sr)
{
    toBeRemoved.Clear();

    ...your unchanged code skipped...

   foreach ( Guid clientId in toBeRemoved )
   {
        try
        {
            subscribers.Remove(clientId);
        }
        catch(Exception e)
        {
            System.Diagnostics.Debug.WriteLine("Unsubscribe Error " + 
                e.Message);
        }
   }
}

...your unchanged code skipped...

public void UnsubscribeEvent(Guid clientId)
{
    toBeRemoved.Add( clientId );
}

This will not only solve your problem, it will prevent you from having to keep creating a list from your dictionary, which is expensive if there are a lot of subscribers in there. Assuming the list of subscribers to be removed on any given iteration is lower than the total number in the list, this should be faster. But of course feel free to profile it to be sure that's the case if there's any doubt in your specific usage situation.

@Karl Kieninger 2015-03-26 21:34:12

I'm expecting this to be worth considering if you have a you are working with larger collections. If it small I'd probably just ToList and move on.

@ford prefect 2013-10-04 14:04:10

So a different way to solve this problem would be instead of removing the elements create a new dictionary and only add the elements you didnt want to remove then replace the original dictionary with the new one. I don't think this is too much of an efficiency problem because it does not increase the number of times you iterate over the structure.

@Rezoan 2013-05-29 13:59:41

You can copy subscribers dictionary object to a same type of temporary dictionary object and then iterate the temporary dictionary object using foreach loop.

@sɐunıɔןɐqɐp 2018-06-24 19:09:08

(This post does not seem to provide a quality answer to the question. Please either edit your answer and, or just post it as a comment to the question).

@Mohammad Sepahvand 2012-05-23 19:32:17

You can also lock your subscribers dictionary to prevent it from being modified whenever its being looped:

 lock (subscribers)
 {
         foreach (var subscriber in subscribers)
         {
               //do something
         }
 }

@Steven Magana-Zook 2013-08-14 03:31:18

+1 for citing a core multi-threaded programming technique

@Jon Coombs 2014-04-21 05:25:23

Is that a complete example? I have a class (_dictionary obj below) that contains a generic Dictionary<string,int> named MarkerFrequencies, but doing this didn't instantly solve the crash: lock (_dictionary.MarkerFrequencies) { foreach (KeyValuePair<string, int> pair in _dictionary.MarkerFrequencies) {...} }

@Mohammad Sepahvand 2014-04-21 06:09:44

@JCoombs it's possible that you're modifying, possibly reassigning the MarkerFrequencies dictionary inside the lock itself, meaning that the original instance is no longer locked. Also try using a for instead of a foreach, See this and this. Let me know if that solves it.

@Jon Coombs 2014-07-22 19:45:13

Well, I finally got around to fixing this bug, and these tips were helpful--thanks! My dataset was small, and this operation was just a UI display update, so iterating over a copy seemed best. (I also experimented with using for instead of foreach after locking MarkerFrequencies in both threads. This prevented the crash but seemed to require further debugging work. And it introduced more complexity. My only reason for having two threads here is to enable the user to cancel an operation, by the way. The UI doesn't directly modify that data.)

@BrainSlugs83 2014-09-08 04:39:46

Problem is, for large applications, locks can be a major performance hit -- better off to use a collection in the System.Collections.Concurrent namespace.

@luc.rg.roy 2012-05-23 16:10:21

Actually the problem seems to me that you are removing elements from the list and expecting to continue to read the list as if nothing had happened.

What you really need to do is to start from the end and back to the begining. Even if you remove elements from the list you will be able to continue reading it.

@Zapnologica 2014-07-17 05:48:24

I dont see how that would make any difference? If you removing an element in the middle of the list it would still throw an error as the item it is trying to access cannot be found?

@BrainSlugs83 2014-09-08 04:37:43

@Zapnologica the difference is -- you wouldn't be enumerating the list -- instead of doing a for/each, you'd be doing a for/next and accessing it by integer -- you can definitely modify a list a in a for/next loop, but never in a for/each loop (because for/each enumerates) -- you can also do it going forwards in a for/next, provided you have extra logic to adjust your counters, etc.

Related Questions

Sponsored Content

26 Answered Questions

[SOLVED] How do I enumerate an enum in C#?

2 Answered Questions

[SOLVED] WCF Services issue? (2 way connection)

  • 2009-11-19 08:38:12
  • Rev
  • 940 View
  • 0 Score
  • 2 Answer
  • Tags:   c# .net wcf

9 Answered Questions

3 Answered Questions

How to catch a custom FaultException in WCF

  • 2016-09-07 11:43:18
  • packoman
  • 770 View
  • 0 Score
  • 3 Answer
  • Tags:   c# .net wcf

1 Answered Questions

[SOLVED] How can I determine first call to WCF singleton service?

2 Answered Questions

[SOLVED] Handling dropped clients in a duplex binding WCF application

5 Answered Questions

[SOLVED] C# Collection was modified; enumeration operation may not execute

1 Answered Questions

[SOLVED] Calling WCF Webservice from Excel VBA using GetObject() method

4 Answered Questions

[SOLVED] Trouble with an enumeration as a key in a Dictionary collection

Sponsored Content