Another locking question

Question

I'm trying to get my multithreading understanding locked down. I'm doing my best to teach myself, but some of these issues need clarification.

I've gone through three iterations with a piece of code, experimenting with locking.

In this code, the only thing that needs locking is this.managerThreadPriority.

First, the simple, procedural approach, with minimalistic locking.

var managerThread = new Thread
(
    new ThreadStart(this.ManagerThreadEntryPoint)
);

lock (this.locker)
{
    managerThread.Priority = this.managerThreadPriority;
}

managerThread.Name = string.Format("Manager Thread ({0})", managerThread.GetHashCode());

managerThread.Start();

Next, a single statement to create and launch a new thread, but the lock appears to be scoped too large, to include the creation and launching of the thread. The compiler doesn't somehow magically know that the lock can be released after this.managerThreadPriority is used.

This kind of naive locking should be avoided, I would assume.

lock (this.locker)
{
    new Thread
    (
     new ThreadStart(this.ManagerThreadEntryPoint)
    )
    {
     Priority = this.managerThreadPriority,
     Name = string.Format("Manager Thread ({0})", GetHashCode())
    }
    .Start();
}

Last, a single statement to create and launch a new thread, with a "embedded" lock only around the shared field.

new Thread
(
    new ThreadStart(this.ManagerThreadEntryPoint)
)
{
    Priority = new Func<ThreadPriorty>(() =>
    {
     lock (this.locker)
     {
      return this.managerThreadPriority;
     }
    })(),

    Name = string.Format("Manager Thread ({0})", GetHashCode())
}
.Start();

Care to comment about the scoping of lock statements? For example, if I need to use a field in an if statement and that field needs to be locked, should I avoid locking the entire if statement? E.g.

bool isDumb;

lock (this.locker) isDumb = this.FieldAccessibleByMultipleThreads;

if (isDumb) ...

Vs.

lock (this.locker)
{
    if (this.FieldAccessibleByMultipleThreads) ...
}

Answer 1

1) Before you even start the other thread, you don't have to worry about shared access to it at all.

2) Yes, you should lock all access to shared mutable data. (If it's immutable, no locking is required.)

3) Don't use GetHashCode() to indicate a thread ID. Use Thread.ManagedThreadId. I know, there are books which recommend Thread.GetHashCode() - but look at the docs.

Answer 2

There is no need to lock anything before you have started any threads.

If you are only going to read a variable there's no need for locks either. It's when you mix reads and writes that you need to use mutexes and similar locking, and you need to lock in both the reading and the writing thread.

Answer 3

Care to comment about the scoping of lock statements? For example, if I need to use a field in an if statement and that field needs to be locked, should I avoid locking the entire if statement?

In general, it should be scoped for the portion of code that needs the resource being guarded, and no more than that. This is so it can be available for other threads to make use of it as soon as possible.

But it depends on whether the resource you are locking is part of a bigger picture that has to maintain consistency, or whether it is a standalone resource not related directly to any other.

If you have interrelated parts that need to all change in a synchronized manner, that whole set of parts needs to be locked for the duration of the whole process. If you have an independent, single item uncoupled to anything else, then only that one item needs to be locked long enough for a portion of the process to access it.

Another way to say it is, are you protecting synchronous or asynchronous access to the resource?

Synchronous access needs to hold on to it longer in general because it cares about a bigger picture that the resource is a part of. It must maintain consistency with related resources. You may very well wrap an entire for-loop in such a case if you want to prevent interruptions until all are processed.

Asynchronous access should hold onto it as briefly as possible. Thus, the more appropriate place for the lock would be inside portions of code, such as inside a for-loop or if-statement so you can free up the individual elements right away even before other ones are processed.

---

Aside from these two considerations, I would add one more. Avoid nesting of locks involving two different locking objects. I have learned by experience that it is a likely source of deadlocks, particularly if other parts of the code use them. If the two objects are part of a group that needs to be treated as a single whole all the time, such nesting should be refactored out.