Method Synchronizing Threading Issue

Question

I can't Seem to get a final counter value Of 20000. What is wrong with this code?

public class Synchronize2 {

    public static void main(String[] args) {
        Threading t1 = new Threading();
        Threading t2 = new Threading();

        t1.start();
        t2.start();

        try {
            t1.join();
            t2.join();
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

        System.out.println(Threading.counter);
    }
}


class Threading extends Thread {

    static int counter;

    public synchronized void incrementer() {
        counter++;
    }

    public void run() {
        for (int i=0; i<10000; i++) {
            incrementer();
        }
    }
}

I'd highly recommend using AtomicInt rather than int for this and avoiding most of manual synchronization. — drobert
– drobert, Commented Jan 10, 2014 at 16:54
Also can't entirely remember if 'static' and 'volatile' go together, but I'd imagine 'counter' needs to be 'volatile' so that threads don't hang on to a local-register cached copy of the value — drobert
– drobert, Commented Jan 10, 2014 at 16:55

rgettman · Accepted Answer · 2014-01-10 16:59:06Z

3

Your synchronized incrementer method will lock on the object itself. But you have 2 different objects, each locking on themselves, so the method isn't thread safe; both threads can still access incrementer at the same time.

Additionally, the post-increment operation isn't thread safe because it's not atomic; there is a read operation and an increment operation, and a thread can be interrupted in the middle of the two operations. This non-thread-safe code presents a race condition, where thread one reads the value, thread two reads the value, then thread one increments and thread two increments, yet only the last increment "wins" and one increment is lost. This shows up when the ending value is less than 20000.

Make the method static too, so that because it's synchronized, it will lock on the class object of the class, which is proper synchronization here.

public static synchronized void incrementer() {

edited Jan 10, 2014 at 16:59

answered Jan 10, 2014 at 16:50

rgettman

179k30 gold badges282 silver badges365 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Boris the Spider Over a year ago

Or use an AtomicInteger and drop the synchronized entirely.

Gray Over a year ago

Might be good to explain that because they aren't locking on the same object, the ++ isn't thread safe. If he changes to a static method then this will fix the thread-safety issue.

Matthias · Accepted Answer · 2014-01-10 17:01:15Z

0

You synchronize on two different Objects. Your incrementer is a short form of this:

    public void incrementer() {
        synchronized (this) {
            counter++;
        }
    }

But the two instances of "this" are not the same Object. Thus, you do not synchronize at all. Try it this way:

    private static Object sync = new Object();

    public void incrementer() {
        synchronized (sync) {
            counter++;
        }
    }

You should also make the variable counter volatile. It is not strictly neccessary here, because you use it only in synchronized blocks. But in real code you might read it outside such a block, and then you will get problems. Non volatile variables can be read from a local thread cache, instead from the memory.

answered Jan 10, 2014 at 17:01

Matthias

8372 gold badges7 silver badges15 bronze badges

2 Comments

Boris the Spider Over a year ago

If you're going down the synchronized and volatile route then I would strongly recommend using an AtomicInteger - this is much less likely to cause problems later.

Matthias Over a year ago

Yes, of course you are right. I assumed it was a question regarding threading, not regarding incrementing integers. Thus, I focussed with my answer on the threading part

Collectives™ on Stack Overflow

Method Synchronizing Threading Issue

2 Answers 2

2 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related