Lock scope in C#: is returned object still "locked"?

Question

Assuming I have an object A containing

// ...

private List<double> someList = new List<double>();

// ... 

public List<double> SomeList
{
    get { lock (this) { return someList; } }
}

// ...

would it be thread safe to perform operation on the list as in the code below. Knowing that several operations could be executed simultaneously by different threads.

A.SomeList.Add(2.0);

or

A.SomeList.RemoveAt(0);

In other words, when is the lock released?

You should never lock on an object exposed outside of your class, such as `this`. — svick, Apr 01 '12 at 17:57
Ok fair enough. Assuming I lock on an object not exposed outside. — Pierre-Antoine, Apr 01 '12 at 17:58
Because another class might use it and you might run into a deadlock. — Pierre-Antoine, Apr 01 '12 at 18:07
i see.. BTW: there is also a question on this topic http://stackoverflow.com/questions/251391/why-is-lockthis-bad — surfen, Apr 01 '12 at 18:16

Oded · Answer 1 · 2012-04-01T18:15:03.113

12

There is no thread safety here.

The lock is released as soon as the block it protects is finished, just before the property returns, so the calls to Add ad RemoveAt are not protected by the lock.

edited Apr 01 '12 at 18:15

answered Apr 01 '12 at 17:57

Oded

489,969
99
883
1,009

Thanks that what I though. Is there any common way to make a situation like that thread safe? – Pierre-Antoine Apr 01 '12 at 18:01
@user1162647 - Yes, you lock around the _operations_ on the shared object that you want to protect. – Oded Apr 01 '12 at 18:18

Gebb · Accepted Answer · 2012-04-01T18:08:04.367

6

The lock you shown in the question isn't of much use.

To make list operations thread safe you need to implement your own Add/Remove/etc methods wrapping those of the list.

public void Add(double item)
{
    lock(_list)
    {
        _list.Add(item);
    }
}

Also, it's a good idea to hide the list itself from the consumers of your class, i.e. make the field private.

edited Apr 01 '12 at 18:08

answered Apr 01 '12 at 18:02

Gebb

6,371
3
44
56

Thanks that was what I was fearing. – Pierre-Antoine Apr 01 '12 at 18:03
2

You can't access `SyncRoot` on a `List` like that, because it's an explicit interface implementation. And I think there is no good reason to use it, it's there mostly just for backwards compatibility. – svick Apr 01 '12 at 18:05

score 3 · Answer 3 · answered Apr 01 '12 at 18:00

The lock is released when you exit the body of the lock statement. That means that your code is not thread-safe.

In other words, you can be sure that two threads won't be executing return someList on the same object at the same time. But it's certainly possible that one thread will execute Add() at the same time as another thread will execute RemoveAt(), which is what makes it non thread-safe.

score 2 · Answer 4 · answered Apr 01 '12 at 17:59

2

The lock is released when the code inside the lock is finished executing. Also, locking on this will only affect the current instance of the object

answered Apr 01 '12 at 17:59

TGH

38,769
12
102
135

score 1 · Answer 5 · answered Apr 01 '12 at 18:17

1

Ok, just for the hell of it.
There IS a way to make your object threadsafe, by using architectures that are already threadsafe.

For example, you could make your object a single threaded COM object. The COM object will be thread safe, but you'll pay with performance (the price of being lazy and not managing your own locks).

Create a COM Object in C#

answered Apr 01 '12 at 18:17

Yochai Timmer

48,127
24
147
185

1

This is like killing a fly with a locomotive. It might actually work, but it's more difficult that doing it properly and can introduces side-effects that might bite you later on. – svick Apr 01 '12 at 18:34

score 1 · Answer 6 · edited May 23 '17 at 10:28

...others said already, but just to formalize a problem a bit...

First, lock (this) {...} suggests a 'scope' - e.g. like using (){} - it only locks (this in this case) for variables inside. And that's a 'good thing' :) actually, as if you couldn't rely on that the whole locks/synchronization concept would be very much useless,
lock (this) { return something; } is an oxymoron of a sort - it's returning something that unlocks the very same moment it returns,
And the problems I think is the understanding of how it works. 'lock()' is not 'persisted' in a state of the object, so that you could return it etc. Take a look here how it's implemented How does lock work exactly? - answer explains it. It's more of a 'critical section' - i.e. you protect certain parts of 'code', which uses the variable - not the variable itself. Any type of synchronization requires 'synchronization objects' to hold the locks - and to be disposed of once lock is no longer needed. Take a look at this post https://stackoverflow.com/a/251668/417747, Esteban formulated that very well,

"Finally, there is the common misconception that lock(this) actually modifies the object passed as a parameter, and in some way makes it read-only or inaccessible. This is false. The object passed as a parameter to lock merely serves as a key" (this is a quote)

you either (usually) lock 'private code' of a class method, property... - to synchronize access to something you're doing inside - and access being to a private member (again usually) so that nobody else can access it w/o going through your synchronized code.
Or you make a thread-safe 'structure' - like a list - which is already 'synchronized' inside so that you can access it in a thread-safe manner. But there are no such things (or not used, almost never) as passing locks around or having one place lock the variable, while the other part of the code unlocks it etc. (in that case it's some type of EventWaitHandle that's rather used to synchronize things in between 'distant' code where one fires off on another etc.)
In your case, the choice is I think to go with the 'synchronized structure', i.e. the list that's internally handled,

Lock scope in C#: is returned object still "locked"?

6 Answers6