Race Condition - czy coś jeszcze?

5 wątków puszczanych jednocześnie. Współdzielą jeden obiekt. Każdy z nich najpierw pobiera atrybut współdzielonego obiektu, a następnie zwiększa jego wartość o 1 i nadpisuje. Czynność powtórzona 20 krotnie, więc obiekt zmaglowany przez 5 wątków powinien skończyć z wartością atrybutu większą o 100.
Jak mniemam zdarza się, że np. wątek A pobierze wartość 50 i zanim zapisze ją jako 51 wątek B też pobierze 50 i zapisze 51. Gdy takie przypadki wystąpią to ostateczna wartość atrybutu będzie powiększona o < 100. To mnie nie dziwi. Ale czemu zdarza się, że ostateczna wartość atrybutu jest powiększona o > 100 ?

Drugie pytanie: jak to nareperowac. Dodawanie synchronized do metod nie pomaga...

public class App {
	
	public static void main(String[] args) throws InterruptedException {
		Counter c1 = new Counter();

		Thread th1 = new Thread(new CounterThread(c1), "thread_1");
		Thread th2 = new Thread(new CounterThread(c1), "thread_2");
		Thread th3 = new Thread(new CounterThread(c1), "thread_3");
		Thread th4 = new Thread(new CounterThread(c1), "thread_4");
		Thread th5 = new Thread(new CounterThread(c1), "thread_5");
				
		th1.start();
		th2.start();
		th3.start();
		th4.start();
		th5.start();

		th1.join();
		th2.join();
		th3.join();
		th4.join();
		th5.join();
		
		System.out.println("===");
		System.out.println(c1.getNumber());
		System.out.println("===");

	}

}

public class CounterThread implements Runnable {

	private Counter counter;
	
	public CounterThread(Counter counter) {
		this.counter = counter;
	}
	
	@Override
	public void run() {
		for (int i = 0; i<100; i++) {
			synchronized(this) {
				int numberToSet = counter.getNumber();
				System.out.println(Thread.currentThread().getName() + ": " + numberToSet);
				counter.setNumber(numberToSet + 1);
			}
			
		}
	}
}

public class Counter {

	private int number;
	private boolean isLocked;

	public synchronized int getNumber() {
		this.isLocked = true;
		return number;
	}

	public synchronized void setNumber(int numberToSet) {
		this.number = numberToSet;
		this.isLocked = false;
	}
}

Synchronizujesz na wątku, a nie na liczniku, więc nic dziwnego, że synchronized nie pomaga.
Poza tym zwiększasz licznik 100 razy w każdym wątku, czyli razem 500, o ile dobrze widzę.

Weź podmień może Countera na
private AtomicInteger counter;

Plus to co powiedział @Afish
Ma na myśli, że nie powinieneś robić synchronized(this), tylko synchronized(counter), jeśli już chcesz się trzymać swojej klasy.

synchronized ( counter) zamiast synchronized (this) - to drugie jest zupełnie bez sensu.
tak samo jak synchronized na pojedynczych metodach. Działa. Ale nie pomaga rozwiązać problemu. Bo lock musisz mieć na całej operacji increment.

A co się mogło stać, że czasem ostateczna wartość przekracza wartość liczby iteracji pomnożonej przez wątki?

Co się mogło stać?

Pewnie oszukałeś.

EDIT:

I jeszcze: System.out.println( (w pętli) poważnie zaburza obserwacje - wywal.
Pętla od 1 do 100 to na increment to raczej śmieszna jest - u mnie te pętle kończą się w czasie niemierzalnym - zanim do odpalenia kolejnego wątku dochodzi.

Rób do miliona lub skomplikuj operacje.
Będziesz miał ciekawszą rzecz do zabawy.

W tej chwili to Twój licznik może się cofać, mimo że z pozoru tylko dodajesz :) Pierwszy wątek może odczytać N, pozostałe w międzyczasie ustawią na M > N i przy zapisie pierwszy wątek pomniejsza wartość licznika ustawiając N :D

https://wiki.sei.cmu.edu/confluence/display/java/VNA02-J.+Ensure+that+compound+operations+on+shared+variables+are+atomic

W kwestii "dobrej zmiany" programu wywal to synchronized(this) bo jest zbędne. Po drugie zmień klasę Counter:

class Counter {
  private int number;

  public synchronized int getNumber() {
    return number;
  }

  public synchronized void incNumber(){
    number++;
  }
}

wtedy wątek:

@Override
  public void run() {
    for (int i = 0; i < 100; i++) {
      System.out.println(Thread.currentThread().getName() + ": " + counter.getNumber());
      counter.incNumber();
    }
  }

cs napisał(a):

W kwestii "dobrej zmiany" programu wywal to synchronized(this) bo jest zbędne. Po drugie zmień klasę Counter:

Kopiuj

class Counter {
  private int number;

  public synchronized int getNumber() {
    return number;
  }

  public synchronized void incNumber(){
    number++;
  }
}

wtedy wątek:

Kopiuj

@Override
  public void run() {
    for (int i = 0; i < 100; i++) {
      System.out.println(Thread.currentThread().getName() + ": " + counter.getNumber());
      counter.incNumber();
    }
  }

o i to jest kolejny przykład któremu się dziwię. Jak puszczę to bez synchrinized to i tak działa jak należy, nie umiem tak tym zatrząść by zadziałało źle

@Julian_, @jarekr000000: Zamiast zwiększać limit pętli, można też dać szanse na wykonanie przeplotu w sekcji krytycznej, czyli w tym fragmencie, który musi być synchronizowany np.:

class Counter {

    private int number;

    public int getNumber() {
        return number;
    }

    public synchronized incNumber() throws InterruptedException {
        int curr = number;
        System.out.println("Starting inc " + curr);
        this.number++;
        this.number--;
        Thread.sleep(1);
        this.number++;
        System.out.println("Ending inc " + curr +", now value is " + number);
    }
}

Tutaj usunięcie synchronized "psuje" program na tyle, że na 10 uruchomień 8-9 wynik będzie błędny.

juljan, we no przeczytaj jcip, a nie zadawaj lamerskich pytan;)