Simultaneously field access in a class by many threads. What pattern should
Hi all,
Sorry if it's a newbie question, but I didn't find the path to solve this problem in the forums, so I put this question:
I have many threads that may run simultaneously with need to access field values in a class instance that control some counters. These threads should access these values and update them too.
My class that manage these counters is something like this (I suppose i should be like this) :
publicclass Counter{
privateint min;
privateint max;
privateint current;
public TrataContadores(){
}
publicsynchronizedint getCurrent(){
return current;
}
publicsynchronizedint getMax(){
return max;
}
publicsynchronizedint getMin(){
return min;
}
publicsynchronizedvoid setCurrent(int current){
this.current = current;
}
publicsynchronizedvoid setMax(int max){
this.max = max;
}
publicsynchronizedvoid setMin(int min){
this.min = min;
}
}
Well, what I don't know is how to make an instance of this class visible for all threads coming alive. I thought about make an static reference to an object of this class and all threads would look for this instance. Would it work properly ?
What could be the best practice to implement int this case ?
I don't know if I could be clear in my explanation. I hope so.
Thanks.
# 1
If you have only one instance then Singleton is maybe a good idea. Otherwise you can extend Runnable and pass to the constructor the counter instance.
class MyTask extends Runnable {
private Counter counter;
MyTask(Counter counter) {
this.counter = counter;
}
public void run() {
// do something with the counter
}
}
...
Counter c = new Counter();
// start one thread
new Thread(new MyTask(c)).start();
// start the second one
new Thread(new MyTask(c)).start();
...
Just a small comment, your constructor should be named Counter, not TrataContadores.
# 2
I would definitely recommend passing the Counter in as explained in the last post over a Singleton.
Also, in Java 1.5 and above, you may be able to use volatile for these simple fields. However, if your code requires modifying more than one of these fields at a time in an atomic way (e.g. you need to modify current and max before another thread reads or modifies either one) neither volatile nor the synchronization you have here will work. Do not use volatile in pre-1.5 because the semantics are not really correct and trying to determine whether it will work properly is not worth the effort.
# 3
Thanks beradrian.I'll look for Singleton implementation.About the constructor, this error occurred because i tried to "translate" some names from portuguese, and I forgot the constructor. ;-)Thanks.
# 4
Thanks dubwai.
I'll take a look at singleton implementation.
I believe I understood what you mean about modifying more than one field at same time. If I need to do that, I would do it by calling setters methods. Even this way I would have problems and neither volatile nor sinchronized would solve it ?
# 5
> Thanks dubwai.
>
> I'll take a look at singleton implementation.
Just to be clear, I'm advocating against using Singleton.
> I believe I understood what you mean about modifying
> more than one field at same time. If I need to do
> that, I would do it by calling setters methods. Even
> this way I would have problems and neither volatile
> nor sinchronized would solve it ?
Not the way you have. Synchronizing the method only blocks other threads during the execution of that method. If you call two synchronized methods, each call acquires and releases the lock separately. Say you have one thread that needs to update max and current together and a second thread that needs to read current and then max.
Thread 1 acquires the lock. Sets max.
Thread 1 releases the lock.
Thread 2 acquires the lock. Reads current.
Thread 2 releases the lock.
Thread 1 acquires the lock. Sets current.
Thread 1 releases the lock.
Thread 2 acquires the lock. Reads max.
Thread 2 releases the lock.
The solution is to hold the lock across the entire set of operations that need to occur together. It's easier to understand if you get rid of the syntactic sugar of putting synchronized on the method declaration. When you declare getMax as synchronized, it really compiles to this:
public int getMax()
{
synchronized(this) {
return max;
}
}
The Object you synchronize on is essentially a key to find a lock. Don't think that you are 'synchronizing the object'. Synchronizing on an Object doesn't affect that Object. It only affects other threads that also synchronize on that exact same Object.
# 6
Well, I will make many tests with that, but I believe I won't have many problems. I hope.In fact, just one thread will constantly change min, max and current values. Other threads will mostly access another value that I call a "last" value. Thanks dubwai.
# 7
> In fact, just one thread will constantly change min,
> max and current values. Other threads will mostly
> access another value that I call a "last" value.
You could still run into issues. For example, let's say you find a new max. You set current and then you set max. Now, let's say another thread does something like check to see if current is the same as max, or even worse has code like this:
return foo.getMax() - foo.getCurrent();
The reading thread could get the new current and the old max and return a negative value. This would probably be unexpected.
