views:

2952

answers:

8

In what cases is it necessary to synchronize access to instance members? I understand that access to static members of a class always needs to be synchronized- because they are shared across all object instances of the class.

My question is when would I be incorrect if I do not synchronize instance members?

for example if my class is

public class MyClass {
    private int instanceVar = 0;

    public setInstanceVar()
    {
        instanceVar++;
    }

    public getInstanceVar()
    {
        return instanceVar;
    }
}

in what cases (of usage of the class MyClass) would I need to have methods: public synchronized setInstanceVar() and public synchronized getInstanceVar() ?

Thanks in advance for your answers.

+8  A: 

It depends on whether you want your class to be thread-safe. Most classes shouldn't be thread-safe (for simplicity) in which case you don't need synchronization. If you need it to be thread-safe, you should synchronize access or make the variable volatile. (It avoids other threads getting "stale" data.)

Jon Skeet
A: 

In Java, operations on ints are atomic so no, in this case you don't need to synchronize if all you're doing is 1 write and 1 read at a time.

If these were longs or doubles, you do need to synchronize because it's possible for part of the long/double to be updated, then have another thread read, then finally the other part of the long/double updated.

Outlaw Programmer
Though be careful of the ++ and -- operators, they're not atomic. To be on the safe side, try using classes from java.util.concurrent.atomic.
InverseFalcon
+3  A: 

If you want to make this class thread safe I would declare instanceVar as volatile to make sure you get always the most updated value from memory and also I would make the setInstanceVar() synchronized because in the JVM an increment is not an atomic operation.

private volatile int instanceVar =0;

public synchronized setInstanceVar() { instanceVar++;

}
bruno conde
No need to make the field volatile, but setInstanceVar() does need to be synchronized.
Daniel Spiewak
Or you can use a java.util.concurrent.atomic.AtomicInteger.
InverseFalcon
Daniel: If the getInstanceVar is not synchronized then the volatile makes sure that the value returned is fresh. But that's probably going to be lost on most programmers.
Tom Hawtin - tackline
InverseFalcon: AtomicInteger may cause problems if there are large numbers of these objects. AtomicIntegerFieldUpdater may be better, but tends to give significantly worse performance when you have to use it.
Tom Hawtin - tackline
@Tom, wouldn't you want your get method to be synchronized as well?
James McMahon
+1  A: 

. Roughly, the answer is "it depends". Synchronizing your setter and getter here would only have the intended purpose of guaranteeing that multiple threads couldn't read variables between each others increment operations:

 synchronized increment()
 { 
       i++
 }

 synchronized get()
 {
   return i;
  }

but that wouldn't really even work here, because to insure that your caller thread got the same value it incremented, you'd have to guarantee that you're atomically incrementing and then retrieving, which you're not doing here - i.e you'd have to do something like

  synchronized int {
    increment
    return get()
  }

Basically, synchronization is usefull for defining which operations need to be guaranteed to run threadsafe (inotherwords, you can't create a situation where a separate thread undermines your operation and makes your class behave illogically, or undermines what you expect the state of the data to be). It's actually a bigger topic than can be addressed here.

This book Java Concurrency in Practice is excellent, and certainly much more reliable than me.

Steve B.
+2  A: 

The synchronized modifier is really a bad idea and should be avoided at all costs. I think it is commendable that Sun tried to make locking a little easier to acheive, but synchronized just causes more trouble than it is worth.

The issue is that a synchronized method is actually just syntax sugar for getting the lock on this and holding it for the duration of the method. Thus, public synchronized void setInstanceVar() would be equivalent to something like this:

public void setInstanceVar() {
    synchronized(this) {
        instanceVar++;
    }
}

This is bad for two reasons:

  • All synchronized methods within the same class use the exact same lock, which reduces throughput
  • Anyone can get access to the lock, including members of other classes.

There is nothing to prevent me from doing something like this in another class:

MyClass c = new MyClass();
synchronized(c) {
    ...
}

Within that synchronized block, I am holding the lock which is required by all synchronized methods within MyClass. This further reduces throughput and dramatically increases the chances of a deadlock.

A better approach is to have a dedicated lock object and to use the synchronized(...) block directly:

public class MyClass {
    private int instanceVar;
    private final Object lock = new Object();     // must be final!

    public void setInstanceVar() {
        synchronized(lock) {
            instanceVar++;
        }
    }
}

Alternatively, you can use the java.util.concurrent.Lock interface and the java.util.concurrent.locks.ReentrantLock implementation to achieve basically the same result (in fact, it is the same on Java 6).

Daniel Spiewak
+1  A: 

To simply put it, you use synchronized when you have mutliple threads accessing the same method of the same instance which will change the state of the object/or application.

It is meant as a simple way to prevent race conditions between threads, and really you should only use it when you are planning on having concurrent threads accessing the same instance, such as a global object.

Now when you are reading the state of an instance of a object with concurrent threads, you may want to look into the the java.util.concurrent.locks.ReentrantReadWriteLock -- which in theory allows many threads to read at a time, but only one thread is allowed to write. So in the getter and setting method example that everyone seems to be giving, you could do the following:

public class MyClass{
    private ReentrantReadWriteLock rwl = new ReentrantReadWriteLock();
    private int myValue = 0;

    public void setValue(){
        rwl.writeLock().lock();
        myValue++;
       rwl.writeLock().unlock();
    }

    public int getValue(){
       rwl.readLock.lock();
       int result = myValue;
       rwl.readLock.unlock();
       return result;
    }
}
The problem with ReadWriteLock is it imposes some extra overhead. This overhead is only worthwhile if you have *long* read operations, otherwise you're better off with just a single ReentrantLock.
Daniel Spiewak
A: 

Nice:) I have a question: I have a class that i use as a library, newr ccreating objects of it. Looks like:

...class MyUtilLib{

public static synchronized Rectangle getRectOnScreenCenter(int width, int length){ ...code here... }}//end class Now I can call it from an object like: Rectangle rr = MyUtilLib.getRectOnScreenCenter(5,5); And the question is: Should or must those methods be synchronized? thx //bb

A: 

@Daniel Spiewak: I liked your explanation but shouldn't it 'better' be decalred static as well especially to have Thread-safe public classes ?


private static final Object lock = new Object();     // must be final, better be static also
sactiw