CODEWITHSUNDEEP
c#multithreadinglocking
Carra
Carra

Reputation: 17964

New inside a lock

I noticed the following code from our foreign programmers:

private Client[] clients = new Client[0];

public CreateClients(int count)
{
    lock (clients)
    {
        clients = new Client[count];

        for(int i=0; i<count; i++)
        {
           Client[i] = new Client();//Stripped
        }
     }
 }

It's not exactly proper code but I was wondering what exactly this will do. Will this lock on a new object each time this method is called?

Upvotes: 5

Views: 225

Answers (5)

Micha&#235;l
Micha&#235;l

Reputation: 6734

Use :
private object = new Object();

lock(object){

//your code

}

Upvotes: 1

corsiKa
corsiKa

Reputation: 82579

To answer your question of "I was wondering what exactly this will do" consider what happens if two threads try to do this.

Thread 1: locks on the clients reference, which is `new Client[0]`
   Thread 1 has entered the critical  block
Thread 1: makes a array and assigns it to the clients reference
Thread 2: locks on the clients reference, which is the array just made in thread 1
   Thread 2 has entered the critical block

You know have two threads in the critical block at the same time. That's bad.

Upvotes: 5

Jakub Konecki
Jakub Konecki

Reputation: 46008

This code is wrong - it will lock on a new instance every time it's called.

It should look like that:

private static readonly object clientsLock = new object();
private static string[] Clients = null;

public CreateClients(int count)
{
    if(clients == null)
    {
        lock (clientsLock)
        {
            if(clients == null)
            {
                clients = new string[count];

                for(int i=0; i<count; i++)
                {
                     client[i] = new Client();//Stripped
                }
            }
        }
     }
 }

There's no point in locking every time the method is called - that's why the surrounding if clause.

Upvotes: 1

JaredPar
JaredPar

Reputation: 755219

This lock really does nothing. It locks an instance of an object which is immediately changed such that other threads entering this method will lock on a different object. The result is 2 threads executing in the middle of the lock which is probably not what was intended.

A much better approach here is to use a different, non-changing object to lock on

private readonly object clientsLock = new object();
private Client[] clients = new Client[0];  

public CreateClients(int count) {     
  lock (clientsLock) {         
    clients = new string[count]; 
    ...
  }
}

Upvotes: 1

Stuart
Stuart

Reputation: 66882

I think you're correct to doubt this code!

This code will lock on the previous instance each time - this might be the desired effect, but I doubt it. It won't stop multiple threads from creating multiple arrays.

Upvotes: 0

Related Questions