2

I have the following piece of code that is giving me problems and I would appreciate any help:

private static string CreateOptionString(List<VehicleOption> Options)
{
    StringBuilder returnValue = new StringBuilder();
    foreach (VehicleOption option in Options)
    {
        if (option.OptionStatus == ExtendedResponse.OptionState.Included)
        {
            if (returnValue.Length > 0)
            {
                returnValue.Append(", ");
            }
            returnValue.Append(option.OptionName);
        }
    }
    return returnValue.ToString();
}

My original problem was that I was getting a System.InvalidOperationException: collection was modified error on my foreach loop.

1)I still can't figure out why I would get this error because I don't see anywhere that it is modified.

Someone suggested that I copy the List to a new List and loop through the new List. I did that and it got rid of the InvalidOperationException. However, I tried coping the list 2 different ways and both gave me a System.ArgumentException: Destination array was not long enough. Here are the two ways I tried to copy the list

List<VehicleOption> newOptions = new List<VehicleOption>(Options);

and

List<VehicleOption> newOptions = new List<VehicleOption>();
newOptions.AddRange(Options);

Both of these gave me a System.ArgumentException: Destination array was not long enough.

2)Why would either of these methods give me that exception?

Any help would be appreciated, because I am stumped.

Thanks!

Improve this question
6
  • How is CreateOptionString called? Are you sure nothing is altering the List outside of this method? You can also do List<VehicleOption> newList = new List<VehicleOption>(Options.ToArray()); to try copying the items to a new list. Commented Jun 12, 2012 at 22:45
  • 1
    Maybe Options is modified from another thread during the foreach. Commented Jun 12, 2012 at 22:45
  • What context is this? ASP.NET? WCF? WinForms? Commented Jun 12, 2012 at 22:46
  • @YoryeNathan, yes... Tim's and yours comments are way better than mine... Deleted... and will delete this one to not cause confusion. Commented Jun 12, 2012 at 22:56
  • This is a Windows Service. I will post how the method is called once I get back to work. How could Options be modified since that is the entire method? Is the Options List passed via reference? It is a multi-threaded program though. Commented Jun 13, 2012 at 10:27

3 Answers 3

Reset to default
5

Make sure that no other threads are changing the collection while it is being iterated.

Also, another way to do it would be:

return string.Join(", ", options.Where(
    op => op.OptionStatus == ExtendedResponse.OptionState.Included));

Which is nicer AND (surprisingly) faster than using StringBuilder in this case.

Still, this doesn't solve your problem - which is likely caused by a different thread changing the collection.

My first try would be:

private static string CreateOptionString(List<VehicleOption> Options)
{
    lock (Options)
    {
        return string.Join(", ", options.Where(
            op => op.OptionStatus == ExtendedResponse.OptionState.Included));
    }
}

But of course, if we had any information about other threads messing around with that collection, it would've been easier to provide a better thread-safe solution.

Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

+0. While it is useful suggestion it is unlikely to help for this case as there is still iteration over items that is failing for OP.
I agree, but I also mentioned the issue with threads, that is most likely what is causing the problem. It is like an answer + suggestion. I edited to put the answer before the suggestion, though.
+1 now after the edit (also lock(Options) is not conventional - better to lock on some more tightly controlled object and must be done on the other thread too).
@AlexeiLevenkov I agree, but since we don't have any information about the other thread(s), or if they even exist, as the OP hasn't replied to the threads issue yet, it is difficult to provide a better solution.
It is written on 2.0 so I can't use Linq. :(
|
4

It sounds like the list object referenced by Options is being changed by another thread. This would explain both problems.

  1. Obviously, if another thread changes a list while you are iterating it, you would see a "collection was modified" exception since you can't change a list while its elements are being enumerated.
  2. In the latter case, it sounds like the constructor and AddRange are allocating the array, then filling it from the supplied enumerable. If the list grew between the allocation and when it is enumerated to fill the new array, all of a sudden the allocated storage wouldn't be big enough and you would see this behavior.

You need to engineer some locking mechanism so that this code can block any threads that may be writing to the list while it either processes the elements, or makes a copy for processing later.

Improve this answer

2 Comments

I don't understand how the collection would get modified via another thread. No where in the method does that collection get modified.
Not in that method, but is your application multi-threaded? Might another thread be modifying that list object at the same time?
0

Ok, I ended up solving the problem. I put a lock (on the option list) around the call to the method. This fixed all my issues.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.