Is the following thread-safe?

Question

I have the following code:

public class Info
{
    public int Data { get; set; }
}

public class Updater
{
    private static readonly object lockObject = new object();
    private Info myInfo= new Info();

    public Info MyInfo
    {
        get
        {
            lock (lockObject)
            {
                return myInfo;
            }
        }
    }

    public void UpdateInfo()
    {
        lock (lockObject)
        {
            myInfo.Data = ReadFromExternalDevice();
        }
    }        
}

I have one instance of Updater which is accessed from two separate threads.

Updater updater = new Updater();

Thread #1 periodically calls UpdateInfo().

updater.UpdateInfo();

Thread #2 periodically reads from the Data property of the Info property.

int latestData = updater.MyInfo.Data;

Is the above code in anywhere close to being thread-safe?

[What is this thing you call "thread safe"?](https://blogs.msdn.microsoft.com/ericlippert/2009/10/19/what-is-this-thing-you-call-thread-safe/) — Scott Chamberlain, Jun 28 '16 at 15:21
http://stackoverflow.com/questions/2116957/readerwriterlock-vs-lock — Dmitry Bychenko, Jun 28 '16 at 15:22
I *would* note that performing `ReadFromExternalDevice()` from within the lock is probably a bad idea as it will increase your time in lock, and increases chance of contention. Save it to a local and mutate within the lock. — Glorin Oakenfoot, Jun 28 '16 at 15:25
It is pointless to discuss thread safety without making it clear **exactly what you mean**; what scenario are you trying to guard against? even without the locks there can be no torn reads, for example. Even though it isn't the intended usage, you could achieve everything you want in terms of periodic reading via `volatile` or a forced read via `Thread.VolatileRead`. — Marc Gravell, Jun 28 '16 at 15:28

Dmitry Bychenko · Answer 1 · 2016-06-29T07:16:15.510

2

Imagine that

1st thread obtains MyInfo (and leaves the lock after return myInfo;)
1st thread starts reading MyInfo properties
2nd thread starts writing into MyInfo
1st thread keeps on reading MyInfo (partially changed!) properties

in this scenario the code is not thread safe. Even if MyInfo has just one int field it's not thread safe:

MyInfo info = Updater.Info;

Console.Write(info.Data);

// Here 2nd thread changes the info
Console.Write(info.Data);

Typical implementation uses ReaderWriterLockSlim

https://msdn.microsoft.com/en-us/library/system.threading.readerwriterlockslim(v=vs.110).aspx

when reading 1st thread EnterReadLock() reads up (clones) MyInfo into a local variable (or performs all the required procedures with MyInfo), then ExitReadLock()
when writing 2nd thread EnterWriteLock() writes, then ExitWriteLock()

edited Jun 29 '16 at 07:16

answered Jun 28 '16 at 15:24

Dmitry Bychenko

180,369
20
160
215

As long as `Data` is an int, he shouldn't have to worry about partial writes. Still bad form though, you are correct. – Glorin Oakenfoot Jun 28 '16 at 15:29
1

"reading into local variable" won't help with that reference type. You would need to clone the properties (`Data`) too since Thread2 does not change the _reference_ of `MyInfo`, only the properties inside that instance. – René Vogt Jun 28 '16 at 15:32
@René Vogt: Thank you! You're right it should be highlighted that *reading* means *cloning* in the context – Dmitry Bychenko Jun 28 '16 at 15:38

score 1 · Answer 2 · answered Jun 28 '16 at 15:27

If you only care about reading the Data property then this implementation is... let's say... thread-safe enough.

Read and write operations on 32bit values are atomic in C#, so there will be no strange intermediate value in updater.MyInfo.Data at any point. If you have more complex properties that are updated, then this no longer applies. Thread 2 may have only partially updated these properties while Thread 1 is still reading on the same MyInfo.

But if you try to do something like

if (updater.MyInfo.Data == 5)
    updater.MyInfo.Data = 7;

in different threads, this is no longer guaranteed to behave as expected. As another thread may have changed the value of Data between the check and the assignment.

You may want to have a look at Interlocked.Exchange for a thread-safe way to update a variable.

Is the following thread-safe?

2 Answers2