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

Question

I am refactoring 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

1. Instance constructors are thread safe because until the object has been constructed another thread could not get a reference to the object being constructed.

2. Correct

Answer 2

I am 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 am not sure if there are situations that should be handled because of compiler optimization in .net like in the case of java

Answer 4

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 5

@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;
    }
}

Answer 6

@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 7

No problem, happy to help, thanks for the accept :)

Wasn't sure if you needed the current instance count or count at initialization, it was just a thought I thought I should raise :) Good question.

Answer 8

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 9

@Mike Schall

Thanks for the heads up on interlocked, never knew about that, that may well be an easier option. This would be provided there is not a limit to the number of instances. Otherwise, you would be in the same ballpark as Singleton, where you'd not have a lock on the instantiation process..

Answer 10

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;

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

  private readonly int instanceId; 
}

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)