modify a list while doing foreach inside lock()

Question

atm I do it like this:

lock (LockObj)
 {
     foreach (var o in Oo)
     {
        var val = o.DateActive;
        if (val.AddSeconds(30) < DateTime.Now) Oo.Remove(o);
     }
}

and I get this error:

Collection was modified; enumeration operation may not execute

how this should be done?

Use List.RemoveAll or similar. By the way, is there any reason you are giving the items at later indices a slightly lower chance of survival? That does look pretty strange. I would think you want to evaluate DateTime.Now *once*. — Ani, Jan 31 '11 at 09:27
@Ani my web-site's users ping the server (ajax) at a specific interval of time, when they stop pinging they aren't on-line — Omu, Jan 31 '11 at 09:30
@Ani while evaluation `DateTime.Now` only once is certainly cleaner, it doesn't matter here. But `RemoveAll` is certainly preferable over hand coded loops. — CodesInChaos, Jan 31 '11 at 09:47

Marlon · Accepted Answer · 2011-01-31T09:38:38.870

5

You have to use a regular for loop.

for (int i = 0; i < Oo.Length; ++i)
{
    var val = Oo[i];
    if (val.AddSeconds(30) < DateTime.Now)
    {
        Oo.RemoveAt(i);
        i--; // since we just removed an element
    }
}

The reason you cannot edit a collection with a foreach loop is because foreach uses a readonly IEnumerator of the collection you are iterating.

edited Jan 31 '11 at 09:38

answered Jan 31 '11 at 09:25

Marlon

19,924
12
70
101

But if you Oo.RemoveAt(i), things are going to get very screwy for next i. – Kirk Broadhurst Jan 31 '11 at 09:35
Just decrement i: `Oo.RemoveAt(i--);` – Marlon Jan 31 '11 at 09:37

Srinivas Reddy Thatiparthy · Answer 2 · 2011-01-31T10:26:06.833

2

you can't modify a collection you are enumerating.. to change it get a copy of it and change it.

for(var k in OO.ToList())
.....

or

use count and iterate the collection with index,

for (int i=0;i<OO.Count();i++)
.....

edited Jan 31 '11 at 10:26

answered Jan 31 '11 at 09:27

Srinivas Reddy Thatiparthy

10,985
4
45
68

`for(var k in OO.ToList())` won't compile, and still has the same problem after fixing that syntax error anyway ;) – Olhovsky Jan 31 '11 at 09:29
@TheBigO - did u import the namespace System.Linq? – Srinivas Reddy Thatiparthy Jan 31 '11 at 09:32
@Srinivas Reddy Thatiparthy - I didn't try to compile it. Does that compile with linq? – Olhovsky Jan 31 '11 at 21:25
@TheBigO- yep,ToList() is an extension method,which is present in LINQ. – Srinivas Reddy Thatiparthy Feb 01 '11 at 04:58
@Srinivas Reddy Thatiparthy - But does LINQ also make `for()` statements behave like `foreach()` statements? – Olhovsky Feb 02 '11 at 02:30
No,__for__ and __foreach__ have nothing to do with the LINQ – Srinivas Reddy Thatiparthy Feb 03 '11 at 08:54

Olhovsky · Answer 3 · 2011-01-31T21:19:41.703

1

The problem is not related to the lock.

Use a for() loop instead of foreach().

I can't 100% replace your code because your code provides no hint of what collection type "Oo" is. Neither does the name "Oo". Perhaps one of the evils of var keyword overuse? Or maybe I just can't see enough of your code ;)

int size = Oo.Length();
for(int i = 0; i < size; i++){
    if (Oo[i].AddSeconds(30) < DateTime.Now){
        Oo[i].RemoveAt(i);
        size--; // Compensate for new size after removal.
    }
}

edited Jan 31 '11 at 21:19

answered Jan 31 '11 at 09:25

Olhovsky

5,466
3
36
47

How is the use of `var` evil? – Enigmativity Jan 31 '11 at 12:14
@Enigmativity - I didn't say it was, I said "one of the evils of *var keyword overuse*". – Olhovsky Jan 31 '11 at 21:18
Sorry. Allow me to rephrase. ;-) How can `var` be overused? What's the evil? – Enigmativity Feb 02 '11 at 01:45
@Enigmativity - See this: http://stackoverflow.com/questions/41479/use-of-var-keyword-in-c – Olhovsky Feb 02 '11 at 02:28

score 1 · Answer 4 · answered Jan 31 '11 at 09:27

1

You simply cannot modify the collection if you are iterating with foreach. You have two options, Loop with For instead of foreach or create another Collection and modify that.

answered Jan 31 '11 at 09:27

Pabuc

5,528
7
37
52

CodesInChaos · Answer 5 · 2011-01-31T09:45:55.727

This problem is completely unrelated to locking.

If you add/remove elements from a List all iterators pointing to that list become invalid.

One alternative to using an iterator is manually working with indices. Then you can iterate backwards and remove elements with RemoveAt.

for(int i=Oo.Count-1;i>=0;i--)
{
  var o=Oo[i];
  if (o.DateActive.AddSeconds(30)<DateTime.Now)
    Oo.RemoveAt(i);
}

Unfortunately this native implementation is O(n^2). If you write it in a more complex way where you first assign the elements to their new position and then truncate the list it becomes O(n).

Buf if Oo is a List<T> there is a much better solution. You can use Oo.RemoveAll(o=>o.DateActive.AddSeconds(30)<DateTime.Now). Unfortunately you there is no such extension method on IList<T> by default.

I'd write the code like this:

lock (LockObj)
 {
     DateTime deleteTime=DateTime.Now.AddSeconds(-30);
     Oo.RemoveAll(o=>o.DateActive<deleteTime);
}

As a sidenote I'd personally use UTC times instead of local times for such code.

score 1 · Answer 6 · answered Jan 31 '11 at 09:54

class Program 
{
    static void Main(string[] args)
    {
        List<OOItem> oo = new List<OOItem>();

        oo.Add( new OOItem() { DateActive = DateTime.Now.AddSeconds(-31) });

        lock(LockObj)
        {
              foreach( var item in oo.Where( ooItem => ooItem.DateActive.AddSeconds(30) < DateTime.Now  ).ToArray())
              {
                 oo.Remove(item);
              } 
        }

        Debug.Assert( oo.Count ==  0);
    }
}

public class OOItem
{
    public DateTime DateActive { get; set; }
}

score 1 · Answer 7 · answered Jan 31 '11 at 12:29

I'm going to suggest an approach that avoids messing around with decrementing loop indexes and other stuff that makes code difficult to understand.

I think the best bet is to write a nice query and then do a foreach over the result of turning the query into an array:

var inactives = from o in Oo
                where o.DateActive < DateTime.Now
                select o;

foreach (var o in inactives.ToArray())
{
    Oo.Remove(o);
}

This avoids the issue of the collection changing and makes the code quite a bit more readable.

If you're a little more "functionally" oriented then here's another choice:

(from o in Oo
 where o.DateActive < DateTime.Now
 select o)
     .ToList()
     .ForEach(o => Oo.Remove(o));

Enjoy!

score 0 · Answer 8 · answered Jan 31 '11 at 10:53

0

you can use Parallel.ForEach(oO, val=> { oO.Remove(val); })

Parallel doesn't have the IEnumerator problem !

answered Jan 31 '11 at 10:53

Dargos

229
1
3

would it make any sense to use parallel inside lock() – Omu Jan 31 '11 at 12:00

modify a list while doing foreach inside lock()

8 Answers8