lock statement question

  • Thread starter Thread starter Kurt
  • Start date Start date
K

Kurt

Below is a class that can accessed from multiple threads and I want the class to be thread safe. I have a private timer member whose interval can be changed by different threads. Which is the correct way to define the property below.

Thanks
Kurt

Class1
{
private Timer _timer = new Timer();

// Example 1
// No lock since interval is a numeric value and does not require locking
public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
this._timer.Interval = value;
}
}

// Example 2 Lock the _timer object only
public int TimerInterval
{
get
{
lock(this._timer)
return this._timer.Interval;
}
set
{
lock(this._timer)
this._timer.Interval = value;
}
}

// Example 3 lock the whole class
public int TimerInterval
{
get
{
lock(this)
return this._timer.Interval;
}
set
{
lock(this)
this._timer.Interval = value;
}
}
}
 
Actually to lock correctly there are lots of ways but the most common is to make a static unchanging object and lock that.

so

class1
{
static object objLock = new oject();

..... //then

public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
lock(objLock )
{
this._timer.Interval = value;
}
}
}


That's how i would do it.

Below is a class that can accessed from multiple threads and I want the class to be thread safe. I have a private timer member whose interval can be changed by different threads. Which is the correct way to define the property below.

Thanks
Kurt

Class1
{
private Timer _timer = new Timer();

// Example 1
// No lock since interval is a numeric value and does not require locking
public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
this._timer.Interval = value;
}
}

// Example 2 Lock the _timer object only
public int TimerInterval
{
get
{
lock(this._timer)
return this._timer.Interval;
}
set
{
lock(this._timer)
this._timer.Interval = value;
}
}

// Example 3 lock the whole class
public int TimerInterval
{
get
{
lock(this)
return this._timer.Interval;
}
set
{
lock(this)
this._timer.Interval = value;
}
}
}
 
So your saying to not actually lock the timer or the class but to create a dummy static object and lock that?
Actually to lock correctly there are lots of ways but the most common is to make a static unchanging object and lock that.

so

class1
{
static object objLock = new oject();

.... //then

public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
lock(objLock )
{
this._timer.Interval = value;
}
}
}


That's how i would do it.

Below is a class that can accessed from multiple threads and I want the class to be thread safe. I have a private timer member whose interval can be changed by different threads. Which is the correct way to define the property below.

Thanks
Kurt

Class1
{
private Timer _timer = new Timer();

// Example 1
// No lock since interval is a numeric value and does not require locking
public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
this._timer.Interval = value;
}
}

// Example 2 Lock the _timer object only
public int TimerInterval
{
get
{
lock(this._timer)
return this._timer.Interval;
}
set
{
lock(this._timer)
this._timer.Interval = value;
}
}

// Example 3 lock the whole class
public int TimerInterval
{
get
{
lock(this)
return this._timer.Interval;
}
set
{
lock(this)
this._timer.Interval = value;
}
}
}
 
Kurt said:
Below is a class that can accessed from multiple threads and I want the
class to be thread safe. I have a private timer member whose interval
can be changed by different threads. Which is the correct way to define
the property below.

Thanks
Kurt
Click to expand...
[snip 8<]

The object that you use for locking has nothing to do with what you do
in the locked code. The object is only used to identify the lock, it
doesn't do anything to the object.

When you lock on an object, it doesn't protect the object itself from
access in any way, it only prevents other threads to enter any code that
is locked using the same object.

Here's an example:

class Test {

private object _lockingObject = 42;
private int _data;

public void SetData(int value) {
lock (_lockingObject) { _data = value; }
}

public int Test2() {
lock (_lockingObject) { return _data; }
}

}

Here you have two methods that work with the same data. They use the
same object for locking, so while some thread is using one of the
methods, no other thread can use any of the two methods.

What you use for locking has to be a reference type, that's why the
methods lock on _lockingObject, not _data. If you lock on a value type,
you will be locking on a copy of the value, which makes the lock useless.

Using lock(this) can potentially cause a deadlock, as code somewhere
else can lock on the same object. If you declare an object that is
private, there is no risk for that, as that object isn't reachable from
outside your class.
 
Actually to lock correctly there are lots of ways but the most common is to make a static unchanging object and lock that.

so

class1
{
static object objLock = new oject();

.... //then

public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
lock(objLock )
{
this._timer.Interval = value;
}
}
}

That's how i would do it.
Click to expand...

Hi,

Using a static object to synchronize access to instance members would
be overkill. All instances of class1 would have to wait in line to
execute their own TimerInterval property even if the instances are
completely independent.

Also, locking the property getter would be compulsory in this case
since Timer.Interval could be changed by one thread and read by
another. The reader might not a see the changes by the writer.

Brian
 
Göran Andersson said:
Kurt said:
Below is a class that can accessed from multiple threads and I want the
class to be thread safe. I have a private timer member whose interval can
be changed by different threads. Which is the correct way to define the
property below.
Thanks
Kurt
Click to expand...
[snip 8<]

The object that you use for locking has nothing to do with what you do in
the locked code. The object is only used to identify the lock, it doesn't
do anything to the object.

When you lock on an object, it doesn't protect the object itself from
access in any way, it only prevents other threads to enter any code that
is locked using the same object.

Here's an example:

class Test {

private object _lockingObject = 42;
private int _data;

public void SetData(int value) {
lock (_lockingObject) { _data = value; }
}

public int Test2() {
lock (_lockingObject) { return _data; }
}

}

Here you have two methods that work with the same data. They use the same
object for locking, so while some thread is using one of the methods, no
other thread can use any of the two methods.

What you use for locking has to be a reference type, that's why the
methods lock on _lockingObject, not _data. If you lock on a value type,
you will be locking on a copy of the value, which makes the lock useless.

Using lock(this) can potentially cause a deadlock, as code somewhere else
can lock on the same object. If you declare an object that is private,
there is no risk for that, as that object isn't reachable from outside
your class.
Click to expand...


So if I use the timer as the lock object and use this consistantly
throughout my code. Only 1 thread can change the interval at a given time
correct?
 
You don't need to use the timer object when you lock. But the object used
must remain unchanged thats all, hence my static version of it, you shoudl
also set it readonly.

The bit inside the lock can only be changed by the thread executing it, no
other thread wil be allowed access to it. So put the lock around your set
code, and make sure you access it always via the property. And it will be
thread safe.

Kurt said:
Göran Andersson said:
Kurt said:
Below is a class that can accessed from multiple threads and I want the
class to be thread safe. I have a private timer member whose interval
can be changed by different threads. Which is the correct way to define
the property below.
Thanks
Kurt
Click to expand...
[snip 8<]

The object that you use for locking has nothing to do with what you do in
the locked code. The object is only used to identify the lock, it doesn't
do anything to the object.

When you lock on an object, it doesn't protect the object itself from
access in any way, it only prevents other threads to enter any code that
is locked using the same object.

Here's an example:

class Test {

private object _lockingObject = 42;
private int _data;

public void SetData(int value) {
lock (_lockingObject) { _data = value; }
}

public int Test2() {
lock (_lockingObject) { return _data; }
}

}

Here you have two methods that work with the same data. They use the same
object for locking, so while some thread is using one of the methods, no
other thread can use any of the two methods.

What you use for locking has to be a reference type, that's why the
methods lock on _lockingObject, not _data. If you lock on a value type,
you will be locking on a copy of the value, which makes the lock useless.

Using lock(this) can potentially cause a deadlock, as code somewhere else
can lock on the same object. If you declare an object that is private,
there is no risk for that, as that object isn't reachable from outside
your class.
Click to expand...


So if I use the timer as the lock object and use this consistantly
throughout my code. Only 1 thread can change the interval at a given time
correct?
Click to expand...
 
Hi Kurt,

If I had to pick between your 3 methods, I would go with:
get
{
lock(this._timer)
return this._timer.Interval;
}
set
{
lock(this._timer)
this._timer.Interval = value;
}

However, I would prefer to see:

private object _syncroot = new object();
public int TimerInterval
{
get
{
lock(_syncroot)
return this._timer.Interval;
}
set
{
lock(_syncroot)
this._timer.Interval = value;
}
}

I would have a tough time making a solid argument for why the _syncroot
mehtod is better than the other method, but after years of doing this that's
the way I find the easiest to debug and maintain.
 
You don't need to use the timer object when you lock. But the object used
must remain unchanged thats all, hence my static version of it, you shoudl
also set it readonly.
Click to expand...

Hi,

Static doesn't make the variable unchangeable though. That's what
readonly does. Static just means that there is one and only one copy
of the variable. Since there is one Timer per class it would be
better to have one lock object per class as well.

Brian
 
Why a static unchanging object? The addition of a static variable there would only confuse things, without making anything faster, cleaner, or easier to maintain.

Now an instance variable, _syncroot, is a different story...

--
Chris Mullins, MCSD.NET, MCPD:Enterprise, Microsoft C# MVP
http://www.coversant.com/blogs/cmullins
Actually to lock correctly there are lots of ways but the most common is to make a static unchanging object and lock that.

so

class1
{
static object objLock = new oject();

.... //then

public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
lock(objLock )
{
this._timer.Interval = value;
}
}
}


That's how i would do it.

Below is a class that can accessed from multiple threads and I want the class to be thread safe. I have a private timer member whose interval can be changed by different threads. Which is the correct way to define the property below.

Thanks
Kurt

Class1
{
private Timer _timer = new Timer();

// Example 1
// No lock since interval is a numeric value and does not require locking
public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
this._timer.Interval = value;
}
}

// Example 2 Lock the _timer object only
public int TimerInterval
{
get
{
lock(this._timer)
return this._timer.Interval;
}
set
{
lock(this._timer)
this._timer.Interval = value;
}
}

// Example 3 lock the whole class
public int TimerInterval
{
get
{
lock(this)
return this._timer.Interval;
}
set
{
lock(this)
this._timer.Interval = value;
}
}
}
 
lol oops yep no idea why i said that. I read your comment and thought well
duh then read mine and though DOH! lol
 
would those 2 words make it any slower or worse?
Why a static unchanging object? The addition of a static variable there would only confuse things, without making anything faster, cleaner, or easier to maintain.

Now an instance variable, _syncroot, is a different story...

--
Chris Mullins, MCSD.NET, MCPD:Enterprise, Microsoft C# MVP
http://www.coversant.com/blogs/cmullins
Actually to lock correctly there are lots of ways but the most common is to make a static unchanging object and lock that.

so

class1
{
static object objLock = new oject();

.... //then

public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
lock(objLock )
{
this._timer.Interval = value;
}
}
}


That's how i would do it.

Below is a class that can accessed from multiple threads and I want the class to be thread safe. I have a private timer member whose interval can be changed by different threads. Which is the correct way to define the property below.

Thanks
Kurt

Class1
{
private Timer _timer = new Timer();

// Example 1
// No lock since interval is a numeric value and does not require locking
public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
this._timer.Interval = value;
}
}

// Example 2 Lock the _timer object only
public int TimerInterval
{
get
{
lock(this._timer)
return this._timer.Interval;
}
set
{
lock(this._timer)
this._timer.Interval = value;
}
}

// Example 3 lock the whole class
public int TimerInterval
{
get
{
lock(this)
return this._timer.Interval;
}
set
{
lock(this)
this._timer.Interval = value;
}
}
}
 
Chris said:
Hi Kurt,

If I had to pick between your 3 methods, I would go with:
get
{
lock(this._timer)
return this._timer.Interval;
}
set
{
lock(this._timer)
this._timer.Interval = value;
}

However, I would prefer to see:

private object _syncroot = new object();
public int TimerInterval
{
get
{
lock(_syncroot)
return this._timer.Interval;
}
set
{
lock(_syncroot)
this._timer.Interval = value;
}
}

I would have a tough time making a solid argument for why the _syncroot
mehtod is better than the other method, but after years of doing this that's
the way I find the easiest to debug and maintain.
Click to expand...

I can argue for it. :)

By using a private sync object dedicated for the locking, the risk is
minimal that any changes in the code exposes the object outside the class.

As the object serves no other purpose, there is no reason to make it
available outside the class. There might be a reason to expose an object
that is also used for something else (like a timer), which can cause a
deadlock if it's used for locking in some other code.
 
Making the variable static might, make it a teeny, tinny, non-measurably
amount faster. But I doubt it. I would be surprised if even in a very
contrived case it made any signifigant difference. (Static variables don't
get stored on the Managed Heap - they're stored on the high frequency heap.
That's..... slighly different.).

In fact, I could see it making things signfigantly slower - if you had a
number of instances of the timer class, and they were all sharing the single
static lock class, then you would see un-necessary competition for the lock
across many threads. This would be less than ideal.

--
Chris Mullins, MCSD.NET, MCPD:Enterprise, Microsoft C# MVP
http://www.coversant.com/blogs/cmullins

would those 2 words make it any slower or worse?

Why a static unchanging object? The addition of a static variable there
would only confuse things, without making anything faster, cleaner, or
easier to maintain.

Now an instance variable, _syncroot, is a different story...

--
Chris Mullins, MCSD.NET, MCPD:Enterprise, Microsoft C# MVP
http://www.coversant.com/blogs/cmullins
Actually to lock correctly there are lots of ways but the most common is to
make a static unchanging object and lock that.

so

class1
{
static object objLock = new oject();

..... //then

public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
lock(objLock )
{
this._timer.Interval = value;
}
}
}


That's how i would do it.

Below is a class that can accessed from multiple threads and I want the
class to be thread safe. I have a private timer member whose interval can be
changed by different threads. Which is the correct way to define the
property below.

Thanks
Kurt

Class1
{
private Timer _timer = new Timer();

// Example 1
// No lock since interval is a numeric value and does not require
locking
public int TimerInterval
{
get
{
return this._timer.Interval;
}
set
{
this._timer.Interval = value;
}
}

// Example 2 Lock the _timer object only
public int TimerInterval
{
get
{
lock(this._timer)
return this._timer.Interval;
}
set
{
lock(this._timer)
this._timer.Interval = value;
}
}

// Example 3 lock the whole class
public int TimerInterval
{
get
{
lock(this)
return this._timer.Interval;
}
set
{
lock(this)
this._timer.Interval = value;
}
}
}
 
You don't need to use the timer object when you lock. But the object used
must remain unchanged thats all, hence my static version of it, you shoudl
also set it readonly.

The bit inside the lock can only be changed by the thread executing it, no
other thread wil be allowed access to it. So put the lock around your set
code...
Click to expand...

....AND your get code. Locks do more than ensure that two threads can't
run the same code: they also create memory barriers, so that any
threads taking out the same lock will receive correctly updated
versions of changed values.

If you don't lock around the get code, then there is no guarantee that
another thread that only gets the value but never sets it will ever
see updates to local object state: the other thread may cache the
object's state and never bother refreshing its cache. Attempting to
acquire a lock causes memory to synchronize across threads so that
they all see the same thing, as it were.

At least, that's a bastardized version of what I've read. :-)
 
He's using a static member to lock on as he is handling static data in
the locked code. If you handle instance data in the code, you should use
an instance member for the locking.
 
Ah! i see. So its not a big issue, but better practice to do it your way?
Well i learnt something then, ok thanks :)
 
Not just better practice, it's the way to do it so that it works.

If you lock on a static object when handling instance data, you will be
locking out all threads, not only the ones that is needed. If you lock
Ah! i see. So its not a big issue, but better practice to do it your way?
Well i learnt something then, ok thanks :)
Click to expand...
 
Ah! i see. So its not a big issue, but better practice to do it your way?
Well i learnt something then, ok thanks :)
Click to expand...

Hi,

Using a static object to lock instance data is generally safe, but
inefficient.

Using an instance object to lock instance data is generally safe,
correct, and efficient.

Using an instance object to lock static data is almost always unsafe
and incorrect.

Brian
 
Back
Top