synchronized keyword does not work as expected in java

Question

Edit: I have already found the answer on the stack: https://stackoverflow.com/a/16280842/3319557

I face a problem with synchronization. I have two following methods:

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

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

I test each of those (separately) in many threads. First method behaves as expected, but second (incrementCounter2) is wrong. Can somebody explain why is this happening?

I assume this method is well designed, as I found something lookalike in Java Concurrency in Practice. Snipped from this book:

@ThreadSafe
public class ListHelper<E> {
    public List<E> list = Collections.synchronizedList(new ArrayList<E>());
    ...
    public boolean putIfAbsent(E x) {
        synchronized (list) {
            boolean absent = !list.contains(x);
            if (absent)
                list.add(x);
            return absent;
        }
    }
}

I use monitor from the Object I am modifying, exactly like in book.

Full code here:

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class SynchronizationTest {
    public static final int N_THREADS = 500;
    public static final int N_Loops = 5000;
    private Integer counter = 0;
    Lock l = new ReentrantLock();

    public void incrementCounter0() {
        counter++;
    }

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

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

    public void incrementCounter3() {
        try {
            l.lock();
            counter++;
        } finally {
            l.unlock();
        }
    }

    private interface IncrementStrategy {
        void use(SynchronizationTest t);
    }

    private static class IncrementingRunnable implements Runnable {
        SynchronizationTest synchronizationTest;
        IncrementStrategy methodToUse;

        public IncrementingRunnable(SynchronizationTest synchronizationTest, IncrementStrategy methodToUse) {
            this.synchronizationTest = synchronizationTest;
            this.methodToUse = methodToUse;
        }

        @Override
        public void run() {
            for (int i = 0; i < N_Loops; i++) {
                methodToUse.use(synchronizationTest);
            }
        }

    }

    public void test(IncrementStrategy methodToUse, String methodName) {
        counter = 0;
        Thread[] threads = new Thread[N_THREADS];
        for (int i = 0; i < N_THREADS; i++) {
            threads[i] = new Thread(new IncrementingRunnable(this, methodToUse));
            threads[i].start();
        }
        for (int i = 0; i < N_THREADS; i++) {
            try {
                threads[i].join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
        System.out.println(methodName + " diff than expected " + (counter - N_THREADS * N_Loops));
    }

    public void test() {
        test(t -> t.incrementCounter0(), "incrementCounter0 (expected to be wrong)");
        test(t -> t.incrementCounter1(), "incrementCounter1");
        test(t -> t.incrementCounter2(), "incrementCounter2");
        test(t -> t.incrementCounter3(), "incrementCounter3");
    }

    public static void main(String[] args) {
        new SynchronizationTest().test();
    }

}

I know, that ExecutorService should be used, whole problem can be solved with AtomicLong, but it is not the point of this question.

Output of the code is:

incrementCounter0 (expected to be wrong) diff than expected -1831489
incrementCounter1 diff than expected 0
incrementCounter2 diff than expected -599314
incrementCounter3 diff than expected 0

PS. If I add the field to SynchronizationTest

Object counterLock = new Object();

and change incrementCounter2 to:

public void incrementCounter2() {
    synchronized (counterLock) {
        counter++;
    }
}

Then incremetCounter2 works as expected.

Although this is an appropriate question, might i suggest that you present a short example of what is "not working" in your opinion together with the expected and the actual output. If this question should help anybody else who has the same problems it'll have to be more concise — ParkerHalo, Jul 08 '16 at 07:09
I pasted my code with sample output. Expected is that incrementCounter2 diff than expected -599314 will have 0 instead of random value — mlecz, Jul 08 '16 at 07:25
It's too much information. For a question like this it's important to see on first view what the real question is. — ParkerHalo, Jul 08 '16 at 07:47

score 4 · Answer 1 · answered Jul 08 '16 at 07:06

4

You're synchronizing on different objects

incrementCounter1 synchronizes on this, while incrementCounter2 synchronizes on the counter Integer object itself.

answered Jul 08 '16 at 07:06

Jim Garrison

85,615
20
155
190

I edited the post. Those methods do not run at the same time. I test them separately. – mlecz Jul 08 '16 at 07:24

raphaëλ · Answer 2 · 2016-07-08T07:14:26.630

You are trying to use two lock monitors (assuming counter is an Object, perhaps Integer?)

public class Foo {
    // Uses instance of Foo ("this")
    public synchronized void incrementCounter1() {
        counter++;
    }

    public void incrementCounter2() {
        // uses counter object as lock monitor
        synchronized (counter) {
            counter++; 
        }
    }

}

I am not sure what you are trying to achieve with counter++ as it seems counter is of type Integer?

Few options to fix your problem:

Use a the same lock monitor
You might want to look into AtomicInteger
Use the lock API (e.g., ReentrantReadWriteLock)

I edited the post. Those methods do not run at the same time. I test them separately. — mlecz, Jul 08 '16 at 07:24

score 0 · Answer 3 · answered Jul 08 '16 at 07:30

Hideous.

synchronized void method(...

Synchronizes on the this Object.

  synchronized(object) {
      ...

Synchronizes on object.

Now:

synchronized (counter) {
    ++counter;

must also synchronize on an Object, but counter is a primitive type, an int. What happens, is that counter is boxed in an Integer.

When counter is 0 .. 127 the Integer object retrieved is everytime different, but shared. For say 1234 a new unique Integer object is created, and synchronized has no effect whatsoever. (Integer being immutable.)

I would call this almost a language error, something for FindBugs to find.

synchronized keyword does not work as expected in java

3 Answers3