Instance constructor sets a static member, is it thread safe?

Question

I am re-factoring some code and am wondering about the use of a lock in the instance constructor.

public class MyClass {

    private static Int32 counter = 0;
    private Int32 myCount;

    public MyClass() {

        lock(this) {
            counter++;
            myCount = counter;
        }
    }
}

Please confirm

1. Instance constructors are thread-safe.
2. The lock statement prevents access to that code block, not to the static 'counter' member.

If the intent of the original programmer were to have each instance know its 'count', how would I synchronize access to the 'counter' member to ensure that another thread isn't new'ing a MyClass and changing the count before this one sets its count?

FYI - This class is not a singleton. Instances must simply be aware of their number.

Answer 1

If you are only incrementing a number, there is a special class (Interlocked) for just that...

http://msdn.microsoft.com/en-us/library/system.threading.interlocked.increment.aspx

Interlocked.Increment Method

Increments a specified variable and stores the result, as an atomic operation.

System.Threading.Interlocked.Increment(myField);

More information about threading best practices...

http://msdn.microsoft.com/en-us/library/1c9txz50.aspx

Answer 2

I'm guessing this is for a singleton pattern or something like it. What you want to do is not lock your object, but lock the counter while your are modifying it.

private static int counter = 0;
private static object counterLock = new Object();

lock(counterLock) {
    counter++;
    myCounter = counter;
}

Because your current code is sort of redundant. Especially being in the constructor where there is only one thread that can call a constructor, unlike with methods where it could be shared across threads and be accessed from any thread that is shared.

From the little I can tell from you code, you are trying to give the object the current count at the time of it being created. So with the above code the counter will be locked while the counter is updated and set locally. So all other constructors will have to wait for the counter to be released.

Answer 3

You can use another static object to lock on it.

private static Object lockObj = new Object();

and lock this object in the constructor.

lock(lockObj){}

However, I'm not sure if there are situations that should be handled because of compiler optimization in .NET like in the case of java

Answer 4

@ajmastrean

I am not saying you should use the singleton pattern itself, but adopt its method of encapsulating the instantiation process.

i.e.

• Make the constructor private.
• Create a static instance method that returns the type.
• In the static instance method, use the lock keyword before instantiating.
• Instantiate a new instance of the type.
• Increment the count.
• Unlock and return the new instance.

EDIT

One problem that has occurred to me, if how would you know when the count has gone down? ;)

EDIT AGAIN

Thinking about it, you could add code to the destructor that calls another static method to decrement the counter :D

Answer 5

The most efficient way to do this would be to use the Interlocked increment operation. It will increment the counter and return the newly set value of the static counter all at once (atomically)

class MyClass {

    static int _LastInstanceId = 0;
    private readonly int instanceId; 

    public MyClass() { 
        this.instanceId = Interlocked.Increment(ref _LastInstanceId);  
    }
}

In your original example, the lock(this) statement will not have the desired effect because each individual instance will have a different "this" reference, and multiple instances could thus be updating the static member at the same time.

In a sense, constructors can be considered to be thread safe because the reference to the object being constructed is not visible until the constructor has completed, but this doesn't do any good for protecting a static variable.

(Mike Schall had the interlocked bit first)

Answer 6

I think if you modify the Singleton Pattern to include a count (obviously using the thread-safe method), you will be fine :)

Edit

Crap I accidentally deleted!

I am not sure if instance constructors ARE thread safe, I remember reading about this in a design patterns book, you need to ensure that locks are in place during the instantiation process, purely because of this..

Answer 7

@Rob

FYI, This class may not be a singleton, I need access to different instances. They must simply maintain a count. What part of the singleton pattern would you change to perform 'counter' incrementing?

Or are you suggesting that I expose a static method for construction blocking access to the code that increments and reads the counter with a lock.

public MyClass {

    private static Int32 counter = 0;
    public static MyClass GetAnInstance() {

        lock(MyClass) {
            counter++;
            return new MyClass();
        }
    }

    private Int32 myCount;
    private MyClass() {
        myCount = counter;
    }
}