I can not understand why this code does not work correctly.

import java.util.concurrent.atomic.AtomicInteger; public class Test1 extends Thread { public static AtomicInteger counter = new AtomicInteger(0); synchronized void doWork1() { counter.incrementAndGet(); counter.incrementAndGet(); System.out.println("Thread+++" + " - " + Test1.counter); try { Thread.currentThread().sleep(10); } catch (InterruptedException e) { } } synchronized void doWork2() { counter.decrementAndGet(); counter.decrementAndGet(); System.out.println("Thread---" + " - " + Test1.counter); try { Thread.currentThread().sleep(10); } catch (InterruptedException e) { } } public void run() { for (int i = 0; i < 100; i++) { doWork1(); doWork2(); } } public static void main(String[] args) { Test1 test1 = new Test1(); test1.start(); Test1 test2 = new Test1(); test2.start(); } }
  • Sooner or later there will be a situation in which the output will be an odd number. This, in theory, should not occur, because methods are synchronized, and all operations are atomic. - TeaLocust

3 answers 3

When specifying the synchronized for methods, the corresponding class instance is used as a monitor that is captured by streams. Since you have two different instances of the class Test1 , the threads in principle work independently of each other. In order to solve this problem, it is necessary to use a common monitor for both streams, for example, a separate object:

 private final static Object lock = new Object(); void doWork1() { synchronized(lock) { ... } } void doWork2() { synchronized(lock) { ... } }

Thus, the threads will already wait for the monitor to be released before executing the code inside the synchronized block.

    As noted above, synchronization in the case of non-static methods occurs on the object that calls this method. In this case, there are two different instances of the class Test1 , locks are captured on different objects. You can solve the problem in several ways: either capture a lock for the whole class:

     void doWork1() { synchronized(Test1.class) { ... } }

    either lock the lock on the same object (in this case, you can on the counter )

     void doWork1() { synchronized(counter) { ... } }

    I would prefer the second option.

      incrementAndGet - yes, atomic, but two such statements written one after another are not such

      Look, for example, one thread entered doWork1 and executed the counter.incrementAndGet () operation and at that moment control passed to another thread that starts to execute doWork2 and executes it to the end. at its output you get an odd number.