Collection Modified issue

Nov 18, 2010 at 5:02 PM

I am having an issue with getting a 'Collection was modified' exception inside the Messenger code - the stack trace is as follows.

Message = Collection was modified; enumeration operation may not execute.

Stack Trace:
   at System.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource)
   at System.Collections.Generic.List`1.Enumerator.MoveNextRare()
   at System.Collections.Generic.List`1.Enumerator.MoveNext()
   at GalaSoft.MvvmLight.Messaging.Messenger.CleanupList(IDictionary`2 lists)
   at GalaSoft.MvvmLight.Messaging.Messenger.Cleanup()
   at GalaSoft.MvvmLight.Messaging.Messenger.SendToTargetOrType[TMessage](TMessage message, Type messageTargetType, Object token)
   at GalaSoft.MvvmLight.Messaging.Messenger.Send[TMessage](TMessage message)

I saw that you cloned your list inside of SendToTargetOrType() but not inside of CleanupList(). Do you have any thoughts on the reason I would be getting this exception currently? The Messages gets sent from the non-UI thread, so I am not sure if that is part of the issue.

Coordinator
Nov 21, 2010 at 9:22 PM

Hi,

This sounds like a bug. Can you send me a repro?

Thanks,

Laurent

Feb 10, 2011 at 2:38 PM

Salut Laurent,

I was getting the same error in my multi-threaded program -one thread was sending messages, and the other was creating objects which registered to receive messages.  

I fixed it by adding a lock on "lists" in CleanupLists(), and similarly locking "recipients" in Register(...) ... you already had this lock on UnregisterFromLists

Regards,

      Damian

 

 

Coordinator
Feb 10, 2011 at 3:02 PM

Thanks Damian I will fix the bug and push it with the next preview.

Cheers,

Laurent

Feb 10, 2011 at 11:57 PM

Great thanks - and congrats on Mix

A+

Damian

May 18, 2011 at 5:56 PM

Hi,

Is this change part of any recent drops of code?  I got what I thought was the latest but did not see a fix in there.  I tried to implement the fix on my own, but perhaps I am applying the fix incorrectly because I am still seeing issues.

Thank you,

Josef

Coordinator
May 20, 2011 at 1:22 PM

Hi,

Can you describe the issue you have more precisely? Some locking has been introduced, but I found out that some is still missing and want to make sure that I am not missing anything this time,

Thanks,

Laurent

May 20, 2011 at 6:52 PM

Laurent,

We were getting a InvalidOperationException on a collection being modified during one of the foreach loops in CleanupList().  It was on the line that says

foreach (WeakActionAndToken item in list.Value)

 

The situation was basically that we were doing a lot (couple of thousands) of register's very rapidly.  We were also doing a bunch of send's during those register's.  Our system was generally being hammered by a lot of processing, as this was a stress test that we were doing.  We did see that there was a lock in CleanupList() on the lists object.  The scope of the lock that was already there was on everything in the method except for the if (lists == null) check.  We did not see a corresponding lock on an equivalent object to "lists" in the Register() method.  There is a lock on the _registerLock object, but the equivalent of the "lists" object, which in Register() is "recipients" (a reference to _recipientsStrictAction or _recipientsOfSubclassesAction) does not have a lock.  Perhaps I am misunderstanding what is going on here, but to me this seemed to be an issue.  We ended up putting a lock on the "recipients" object as was suggested by dmehers to encompass all of the code in Register() after "recipients" is set.  This seemed to correct our initial issue.  The reason I wrote my first message was because we were applying the lock incorrectly (I think).  I don't remember how we were locking in Register(), but the lock I explained earlier seems to have fixed our issue.

But then we ran into another exception.  We got another InvalidOperationException on a collection being modified, but this time it was in the SendToList() method.  This was on the line that says 

foreach (WeakActionAndToken item in list)

This seemed to be occurring when SendToList() is called based on _recipientsStrictAction.  We don't use any messaging that includes the subclasses, so it may be an issue there as well but we didn't see it through our tests.  What we ended up doing to fix this was removing the listClone in SendToList().  Instead, we ended up putting locks around list = _recipientsOfSubclassesAction[type]; and List<WeakActionAndToken> list = _recipientsStrictAction[messageType]; in the SendToTargetOrType() method by locking the _recipientsOfSubclassesAction and _recipientsStrictAction objects respectively.  Inside of those locks, we performed the list cloning using Take() to replace the listClone that we removed from SendToList().

I hope this makes sense and that it doesn't introduce other errors.  Based on our testing over the last couple days, these two fixes corrected any exceptions that we had been seeing.  If it would help that I send you our version of the Messenger class, please let me know.  I guess I could also post the two methods (Register() and SendToTargetOrType()) that we changed in its entirety...  I will try to remember to do that.

I hope this helps.  MVVM Light has helped us out immensely, and we owe a lot to the community and to Laurent in particular.  Thanks!

May 20, 2011 at 7:28 PM

Here is the code for Register()

        public virtual void Register<TMessage>(
            object recipient,
            object token,
            bool receiveDerivedMessagesToo,
            Action<TMessage> action)
        {
            lock (_registerLock)
            {
                Type messageType = typeof(TMessage);

                Dictionary<Type, List<WeakActionAndToken>> recipients;

                if (receiveDerivedMessagesToo)
                {
                    if (_recipientsOfSubclassesAction == null)
                    {
                        _recipientsOfSubclassesAction = new Dictionary<Type, List<WeakActionAndToken>>();
                    }

                    recipients = _recipientsOfSubclassesAction;
                }
                else
                {
                    if (_recipientsStrictAction == null)
                    {
                        _recipientsStrictAction = new Dictionary<Type, List<WeakActionAndToken>>();
                    }

                    recipients = _recipientsStrictAction;
                }

                lock (recipients) //The lock we added
                {

                    List<WeakActionAndToken> list;

                    if (!recipients.ContainsKey(messageType))
                    {
                        list = new List<WeakActionAndToken>();
                        recipients.Add(messageType, list);
                    }
                    else
                    {
                        list = recipients[messageType];
                    }

                    var weakAction = new WeakAction<TMessage>(recipient, action);
                    var item = new WeakActionAndToken
                    {
                        Action = weakAction,
                        Token = token
                    };
                    list.Add(item);
                }
            }

            Cleanup();
        }

And here is the code for SendToTargetOrType()

        private void SendToTargetOrType<TMessage>(TMessage message, Type messageTargetType, object token)
        {
            Type messageType = typeof(TMessage);

            if (_recipientsOfSubclassesAction != null)
            {
                // Clone to protect from people registering in a "receive message" method
                // Bug correction Messaging BL0008.002
                List<Type> listClone =
                    _recipientsOfSubclassesAction.Keys.Take(_recipientsOfSubclassesAction.Count()).ToList();

                foreach (Type type in listClone)
                {
                    List<WeakActionAndToken> list = null;

                    if (messageType == type
                        || messageType.IsSubclassOf(type)
                        || Implements(messageType, type))
                    {
                        lock (_recipientsOfSubclassesAction)  //The lock we added
                        {
                            //The list we cloned
                            list = _recipientsOfSubclassesAction[type].Take(_recipientsOfSubclassesAction[type].Count()).ToList();
                        }
                    }

                    SendToList(message, list, messageTargetType, token);
                }
            }

            if (_recipientsStrictAction != null)
            {
                if (_recipientsStrictAction.ContainsKey(messageType))
                {
                    List<WeakActionAndToken> list = null;
                    lock (_recipientsStrictAction)  //The lock we added
                    {
                        //The list we cloned
                        list = _recipientsStrictAction[messageType].Take(_recipientsStrictAction[messageType].Count()).ToList();
                    }
                    SendToList(message, list, messageTargetType, token);
                }
            }

            Cleanup();
        }

Coordinator
Jun 2, 2011 at 12:46 PM

Just following up on that. I am introducing these fixes in the code this weekend. I want to take the occasion to correct another bug which is trickier. Once I am done, i plan to push the fix in V3 too, not just V4.

Cheers,

Laurent

Sep 30, 2011 at 1:38 PM
lbugnion wrote:

Just following up on that. I am introducing these fixes in the code this weekend. I want to take the occasion to correct another bug which is trickier. Once I am done, i plan to push the fix in V3 too, not just V4.

Hello,
I analyzed the version released on 1 September 2011 [v3]. From the source code [v3code] it looks like the fix was pushed to V3 (thanks) but I cannot find it in the binary [v3msi] (reading the source code from the dll via reflector).
I also noted that, in [v3msi], GalaSoft.MvvmLight.WPF4.dll declares versione 3.0.0.28535 (that is older of my local "outdated" dll, that is 29166).
Can you please double check the binary released at [v3msi] at the page [v3]?
Cheers
Luca

[v3] http://mvvmlight.codeplex.com/releases/view/71278
[v3code] http://mvvmlight.codeplex.com/SourceControl/changeset/view/ab7442f70691
[v3msi] http://mvvmlight.codeplex.com/releases/71278/download/267119