c# foreach with locked property

Question

Alright, so here is the property I have

public List<String> names{
    get{
        lock(_names)
            return _names;
       }
   set{
       lock(_names)
           _names = value
      }
}
private List<String> _names;

and now say I do a foreach on names like this

foreach(String s in names)
{
    Console.WriteLine(s);
}

My question is, is names locked through the whole foreach, or does it only lock each time s is set, then unlocks inside the foreach.

If that's confusing, say I try to do this

foreach(String s in names)
{
    lock(names)
        Console.WriteLine(s);
}

Will I end up in a deadlock?

score 4 · Answer 1 · edited May 23 '17 at 10:24

4

lock(_names) return _names;

is short for:

try
{
  Monitor.Enter(_names);
  return _names;
}
finally
{
  Monitor.Exit(_names);
}

which is the equivalent of:

Monitor.Enter(_names);
Monitor.Exit(_names);
return _names;

seeing it that way should make it more clear that the lock is not necessary. You might be interested in using one of the thread-safe collections

edited May 23 '17 at 10:24

Community

1
1

answered Jan 08 '12 at 00:26

gordy

9,360
1
31
43

That's not strictly true. It's actually the same as `try { Monitor.Enter(_names); return _names; } finally { Monitor.Exit(_names); }` – Ry- Jan 08 '12 at 00:30
2

Actually I don't think that's correct, _names will be copied to a temporary before the Monitor.Exit is executed, otherwise something could have changed _names after the exit and before the return. – Joachim Isaksson Jan 08 '12 at 00:30
Right, and since the temp var serves no other purpose other than to be returned after the exit my simplified version is accurate. Thanks for the downvote though :\ – gordy Jan 08 '12 at 01:01
kelton52, I was responding to @Joachim Isaksson assuming he downvoted me as he misunderstood my answer. Anyways I edited it for clarity in case anyone else thought my answer was wrong or actually negatively contributed to this discussion. – gordy Jan 08 '12 at 01:28
...and I always vote back up when I downvoted something and it's later clarified. There you go. – Joachim Isaksson Jan 08 '12 at 08:04

Ry- · Answer 2 · 2012-01-08T00:29:25.870

3

It's not locked inside the foreach. It's locked once, to get a reference, then iterated. (Also, your lock statements inside the property are currently unnecessary; those operations are atomic.)

Most likely, you should be locking outside the body of the loop, and where the collection is modified.

In response to your edit, no. That's not a parallel foreach so it's impossible for the loop to get stuck just by itself, let alone deadlock. Once again, that would just be wasted processing power.

edited Jan 08 '12 at 00:29

answered Jan 08 '12 at 00:23

Ry-

218,210
55
464
476

Because the code I have is an example, it had nothing to do with weather or not it the code was useful. – Kelly Elton Jan 12 '12 at 07:50
1

@kelton52: I think your downvote is unfair. It is irrelevant whether your code was an example or not. minitech is just giving you feedback on the code you presented in your question. And it is good feedback. – Steven Jan 12 '12 at 08:28
Ok so maybe it's not obvious that there is no purpose in locking unthreaded code. – Kelly Elton Jan 12 '12 at 09:02

score 3 · Answer 3 · answered Jan 08 '12 at 00:28

3

In this scenario your lock is effectively doing nothing. The getter roughly expands into the following

public List<string> names {
  get {
    Monitor.Enter(_names); 
    try {
      return _names;
    } finally {
      Monitor.Exit(_names);
    }
  }
}

This means access to the resource is synchronized only for the time it takes to read the reference. It will do nothing to protect the integrity of the List<string>.

The names reference is only read once in the foreach so it won't be locked during the body . It won't lead to any deadlocks but it won't do much for protection either

answered Jan 08 '12 at 00:28

JaredPar

733,204
149
1,241
1,454

I wouldn't say it does nothing, I can effectivly say that when I get names that the list won't be changed while I'm grabbing it. – Kelly Elton Jan 08 '12 at 00:31
1

@kelton52 reference reads are already atomic in .Net. Using a lock to guard property read / writes offers you nothing in terms of data integrity or synchronization – JaredPar Jan 08 '12 at 00:33
so you're saying that it's ambiguous, or that it's a bad way to do things? – Kelly Elton Jan 08 '12 at 00:37
@kelton52 mainly i'm saying it provides no discernible value over a scenario where you didn't lock. A lock in .net is generally used for synchronization over a shared resource often to provide data integrity guarantees. Here the only thing the lock is protecting is the reference stored in the field. This is already has atomic read / write guarantees so there is nothing to protect from corruption – JaredPar Jan 08 '12 at 00:39
2

@kelton52 => The question about locks notwithstanding, returning the backing field does not prevent the caller from making changes to your List that the owning class is unaware off. You might consider having your getter return a ReadOnlyCollection instead. – Gayot Fow Jan 08 '12 at 00:58
It gets called by threads, or I wouldn't be locking. My questions were about locking, that's obviously not my whole project. – Kelly Elton Jan 08 '12 at 01:19

score 3 · Answer 4 · answered Jan 08 '12 at 00:33

It's locked only one inside _get method of the property:

    void Main()
    {
        Test test = new Test();
        test.CallMe();
    }


    public class Test {

        List<string> _names= new List<string>(); 

        public List<string> Names {
            get {
            Console.WriteLine("Lock");
            lock(_names) {
                Console.WriteLine("Exit");
                return _names;
            }

            }       

        }

        public void CallMe()
        {
            foreach(String s in Names)
            {
                Console.WriteLine(s);
            }
        }
 }

An output if this is

Lock
Exit

IL Code clearly shows that lock happens inside _get method of the property :

IL_0000:  newobj      UserQuery+Test..ctor
IL_0005:  stloc.0     
IL_0006:  ldloc.0     
IL_0007:  callvirt    UserQuery+Test.CallMe

Test.get_Names:        //GET METHOD OF THE PROPERTY
IL_0000:  ldstr       "Lock"
IL_0005:  call        System.Console.WriteLine
IL_000A:  ldc.i4.0    
IL_000B:  stloc.0     
IL_000C:  ldarg.0     
IL_000D:  ldfld       UserQuery+Test._names
IL_0012:  dup         
IL_0013:  stloc.2     
IL_0014:  ldloca.s    00 
IL_0016:  call        System.Threading.Monitor.Enter  //LOCK
IL_001B:  ldstr       "Exit"
IL_0020:  call        System.Console.WriteLine
IL_0025:  ldarg.0     
IL_0026:  ldfld       UserQuery+Test._names
IL_002B:  stloc.1     
IL_002C:  leave.s     IL_0038
IL_002E:  ldloc.0     
IL_002F:  brfalse.s   IL_0037
IL_0031:  ldloc.2     
IL_0032:  call        System.Threading.Monitor.Exit //UNLOCK
IL_0037:  endfinally  
IL_0038:  ldloc.1     
IL_0039:  ret         

Test.CallMe:     // CALLME METHOD CODE
IL_0000:  ldarg.0     
IL_0001:  call        UserQuery+Test.get_Names //ONCE !!
IL_0006:  callvirt    System.Collections.Generic.List<System.String>.GetEnumerator
IL_000B:  stloc.1     
IL_000C:  br.s        IL_001C
IL_000E:  ldloca.s    01 
IL_0010:  call        System.Collections.Generic.List<System.String>.get_Current
IL_0015:  stloc.0     
IL_0016:  ldloc.0     
IL_0017:  call        System.Console.WriteLine
IL_001C:  ldloca.s    01 
IL_001E:  call        System.Collections.Generic.List<System.String>.MoveNext
IL_0023:  brtrue.s    IL_000E
IL_0025:  leave.s     IL_0035
IL_0027:  ldloca.s    01 
IL_0029:  constrained. System.Collections.Generic.List<>.Enumerator
IL_002F:  callvirt    System.IDisposable.Dispose
IL_0034:  endfinally  
IL_0035:  ret         

Test..ctor:
IL_0000:  ldarg.0     
IL_0001:  newobj      System.Collections.Generic.List<System.String>..ctor
IL_0006:  stfld       UserQuery+Test._names
IL_000B:  ldarg.0     
IL_000C:  call        System.Object..ctor
IL_0011:  ret

Steven · Accepted Answer · 2012-01-12T08:21:48.290

2

The get_Names method will only get called once, and the lock will be over (there will be no lock) during the iteration of the items. This is probably not what you intend and you should probably go for a much granular lock, such as:

lock (someLockObject)
{
    foreach(String s in names)
    {
        Console.WriteLine(s);
    }
}

When you lock inside the loop, you will take many locks one after the other, which is almost certainly not what you want, since operating the foreach loop will not be atomic anymore.

edited Jan 12 '12 at 08:21

answered Jan 08 '12 at 00:23

Steven

166,672
24
332
435

Is that also the case if I do `names.ForEach` ? – Kelly Elton Jan 08 '12 at 00:24
@kelton52: yes. Just think of what happens: 1. You call the `get_names` method. 2. It starts a lock. 3. Puts the return value on the stack. 4. ends the lock. 5. Method returns. 6. The `ForEach()` method is called on that returned value (outside the lock). – Steven Jan 08 '12 at 00:26
What about Linq, if I did a Where or a FirstOrDefault? – Kelly Elton Jan 08 '12 at 00:26
Now, for one last question, if my example, I `lock` `_names`, if I do a lock on `names` will that create a deadlock? – Kelly Elton Jan 08 '12 at 00:27
1

@kelton52: With LINQ, the result is the same. Let's make this clear: *you are not synchronizing the list by doing that*. – Ry- Jan 08 '12 at 00:30
1

@kelton52: A `lock(_names)` will not prevent others from using `_names`. The `lock` keyword uses the passed in object as a *key* and when other threads call `lock`, it will make sure that no other thread has locked on that key (and if so, it will wait till the other thread finished his lock). You can access and change `_names` within the same thread without a problem, since `_names` is not really locked. – Steven Jan 08 '12 at 00:31
Ok, I'm just trying to get a good understanding of locking so I can make sure I do things right. – Kelly Elton Jan 08 '12 at 00:33
wait though, one thing. I'm always accessing `names` not `_names`, so that it locks. I never access `_names` directly so it would be protected then? Correct? – Kelly Elton Jan 08 '12 at 00:34
1

@kelton52: Let's make this clear again: No. Not correct. Neither of them are protected. *You are not synchronizing the list by locking your properties.* You are not synchronizing anything that is not already synchronized. – Ry- Jan 08 '12 at 00:36
well every time I access `names` it locks on `_names`, so noone else can enter the get/set method of `names` until the lock releases. SO if that's the case, then if I do a get to get the contents, nothing can be added to the list, since I only make calls to the property `names` and not the variable `_names` – Kelly Elton Jan 08 '12 at 00:39
@kelton52: That statement is completely wrong, unfortunately. – Ry- Jan 09 '12 at 17:40

score 2 · Answer 6 · answered Jan 08 '12 at 00:26

The foreach loop locks _names briefly to get the List<>, but unlocks before it starts the actual loop.

Locking on _names may not be exactly good form, I'd introduce another object to lock on that isn't changed by the actual set operation that's supposedly locked.

c# foreach with locked property

6 Answers6