C# threading and synchronization

Question

I have a private static field which I use for synchronization (lock). Now I have two functions which I don't want to execute concurrently. So I did this:

public class Synchronization
{
    private static object _lock = new object();

    public void MethodA()
    {
        lock (_lock)
        {
            Console.WriteLine("I shouldn't execute with MethodB");
        }
    }

    public void MethodB()
    {
        lock (_lock)
        {
            Console.WriteLine("I shouldn't execute with MethodA");
        }
    }
}

I know that locking on an object will prevent parallel execution of a single function, but will the same work if I use the same lock object in different methods which run concurrently? Simply, put can any other thread acquire lock on an already locked object in another function?

Also note that this way, you've synchronized EVERY instance of the class. If you don't want that, remove the "static" part of the lock object. — Willem van Rumpt
– Willem van Rumpt, Commented Jan 26, 2011 at 9:30
@Andreas I said theory and practice. Verifying MT code is also notoriously difficult. So your suggestion is naive. — Tim Lloyd
– Tim Lloyd, Commented Jan 26, 2011 at 9:36
@Andreas "Simply give it a try yourself" does not give the OP details of how to verify his assertions. If the OP is having difficulty working out the semantics of what he is doing, then you should be able to see the obvious problem with your suggestion. It's a throw away comment. — Tim Lloyd
– Tim Lloyd, Commented Jan 26, 2011 at 9:39
@Andreas Sure: "the internet never forgets". Any questions, answers, and comments that are left here should be useful for other users of the site, and preferably give nice juice for search engines. the OP is asking how you actually follow up on your comment i.e. "How can I test this? – Tux" — Tim Lloyd
– Tim Lloyd, Commented Jan 26, 2011 at 9:46

Tim Lloyd · Accepted Answer · 2011-01-26 09:49:18Z

5

Only a single thread at a time may acquire the lock, so this state is exclusive for all threads on a single lock instance. So in your example, only one method body may be executing at any given time for all instances of class Synchronization as your lock is static. If you want locking per instance of your class, then do not mark the lock object as static.

Your assumptions regarding synchronization are correct.

Please note that you should mark the lock object readonly for a completely water-tight solution. As the code stands, it would be possible for the lock object to be re-assigned and so break the locking semantics, e.g.:

public class Synchronization
{
    private static object _lock = new object();

    public void MethodA()
    {
        lock (_lock)
        {
            Console.WriteLine("I shouldn't execute with MethodB");
        }
    }

    public void MethodB()
    {
        //This shouldn't be allowed!
        _lock = new object();

        lock (_lock)
        {
            Console.WriteLine("I shouldn't execute with MethodA");
        }
    }
}

Lock object should be marked as readonly, i.e.:

private static readonly object _lock = new object();

edited Jan 26, 2011 at 9:49

answered Jan 26, 2011 at 9:29

Tim Lloyd

38.6k10 gold badges106 silver badges132 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

user57508 Over a year ago

you should enhance only one method body with of any given instance, as you are dealing with a static lock :)

Tim Lloyd Over a year ago

@Andreas I have detailed the implication of the static lock.

Ian · Accepted Answer · 2011-01-26 09:28:42Z

2

I believe you're correct reading the Lock Statement on MSDN.

answered Jan 26, 2011 at 9:28

Ian

34.8k30 gold badges127 silver badges211 bronze badges

Comments

GvS · Accepted Answer · 2011-01-26 09:29:11Z

1

You are doing it correctly. You have created two critical sections that will not be entered on the same time.

So MethodA and MethodB will not be "active" at the same time. And also there will be only one MethodA (and MethodB) active at the same time.

This is valid for across all objects that you create. What I mean is: only one thread will be in any MethodA or MethodB from any object. If you want the lock to occur only within one object, you can make the _lock object not static.

answered Jan 26, 2011 at 9:29

GvS

52.5k16 gold badges104 silver badges139 bronze badges

Comments

marcind · Accepted Answer · 2011-01-26 09:30:09Z

1

Locks are granted based on the object that is the target of the lock, not the method in which the lock statement occurs. So in your case you multiple thread might enter the various methods but only one thread at a time will be able to execute any code that's within the lock statement.

answered Jan 26, 2011 at 9:30

marcind

53.3k13 gold badges129 silver badges112 bronze badges

Comments

František Žiačik · Accepted Answer · 2011-01-26 09:46:26Z

1

First, the _lock should not be static. Or do you want multiple instances of the object to lock themselves each other? Second, you should have only one synchronized method in a class. Even more, you should avoid dependencies between synchronized methods in your class. Otherwise, you are in a risk that the caller of your methods will do it wrong and get unexpected behavior.

Consider, for example, this code:

class Synchronized
{
    object lockObj = new object();
    int counter = 100;

    public void Decrement()
    {
        lock (this.lockObj)
        {
            this.counter--;
        }
    }

    public int IsZero()
    {
        lock (this.lockObj)
        {
            return this.counter == 0;
        }
    }
}

Now what will one do with shared Synchronized instance?

Use it like this

while (!synchronized.IsZero())
{
    synchronized.Decrement();
}

Now thread 1 calls Decrement, counter gets to 0 and immediately thread 2 calls Decrement, because it was waiting on lock in Decrement method, not IsZero method. Counter is now -1 and the loop is infinite.

It's not that the locking mechanism was incorrectly programmed, but that the caller didn't use it well. If you only exposed one synchronized method on your Synchronized class, you wouldn't fool the programmer to blindly believe it's safe.

It should be something like this:

class Synchronized
{
    object lockObj = new object();
    int counter = 100;

    public bool IfNotZeroDecrement()
    {
        lock (this.lockObj)
        {
            if (this.counter > 0)
                this.counter--;

            return this.counter > 0;
        }
    }    
}

/// Usage:
while (synchronized.IfZeroDecrement())
{
}

answered Jan 26, 2011 at 9:46

František Žiačik

7,6041 gold badge38 silver badges61 bronze badges

3 Comments

btlog Over a year ago

I don't agree with the one syncronised method in a class. Your example is just poor implementation. If it is important that you can't decrement less than 0 your decrement should deal with that.

František Žiačik Over a year ago

@btlog Well it's obvious in this easy example, but in more complicated scenarios it couldn't be that obvious. You shouldn't put all responsibility on the users of your code, otherwise you can get into serious troubles later which are hard to debug. Do it safe and clean and everyone will benefit from that in future.

František Žiačik Over a year ago

Recommended reading about this (and others): blog.objectmentor.com/articles/2008/04/08/clean-code-whew "Dependencies Between Methods Can Break Concurrent Code"

Collectives™ on Stack Overflow

C# threading and synchronization

5 Answers 5

2 Comments

Comments

Comments

Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

2 Comments

Comments

Comments

Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related