Wątki - synchronized

1

Mam taki programik:

public class Main {

	public static void main(String[] args) {
		ExecutorService ex = Executors.newFixedThreadPool(5);
		
		Account accountFrom = new Account(10);
		Account accountTo = new Account(5);
		
		ex.execute(new AccountTask(accountFrom, accountTo));
		ex.execute(new AccountTask(accountFrom, accountTo));
		ex.execute(new AccountTask(accountFrom, accountTo));
	}

}
public class Account {
	
	private int amount;

	public Account(int amount) {
		this.amount = amount;
	}
	
	public void add(int i) {
		amount += i;
	}
	
	public void reduce(int i) {
		amount -= i;
	}
	
	public int getAmount() {
		return amount;
	}

	@Override
	public String toString() {
		return "amount=" + amount;
	}
	
}
public class AccountTask implements Runnable{
	
	private Account accountFrom;
	private Account accountTo;
	AtomicInteger counter = new AtomicInteger(1);
	
	public AccountTask(Account accountFrom, Account accountTo) {
		this.accountFrom = accountFrom;
		this.accountTo = accountTo;
	}

	@Override
	public void run() {
		while(true) {
			synchronized(this) {
				int amo = counter.incrementAndGet();
				
				accountFrom.reduce(amo);
				accountTo.add(amo);
			
				accountTo.reduce(amo);
				accountFrom.add(amo);
				
				if(accountFrom.getAmount() != 10 || accountTo.getAmount() != 5) {
					System.out.println(amo + " " + accountFrom + " " + accountTo);
					break;
				}
			}
		}
	}

}

Po uruchomieniu zawsze udaje mu się wejść do:

if(accountFrom.getAmount() != 10 || accountTo.getAmount() != 5) {
					System.out.println(amo + " " + accountFrom + " " + accountTo);
					break;
				}

pomimo moich nalegań, żeby tego nie robił:

synchronized(this)

Pytanie dlaczego on mi to czyni? W założeniu do obu stron dodaję i odejmuję tyle samo więc ten if nie powinien być osiągalny.

0

No dobra, ale synchronized(this) dotyczy konkretnego Runnable, a Ty odpalasz trzy różne Runnable.

0

Po pierwsze synchronizuj na obiekcie, który jest współdzielony przez wątki, czyli AccountFrom i AccountTo.
Po drugie robienie takich wątków, które tylko wykonują kod synchronizowany jest bez sensu, praktycznie każdy wątek wykona się sekwencyjnie, w danej chwili wykonuje się tylko jeden wątek, tylko kolejność jest przypadkowa.
Po trzecie lepiej synchronizować metody klasy Account, w których modyfikujesz pola np.:

public synchronized void reduce(int i) {
        amount -= i;
    }

Wtedy Twój obiekt, do którego dostają się wątki działa jak prawdziwy monitor, a kod wątku się upraszcza:

@Override
    public void run() {
        while(true) {
                int amo = counter.incrementAndGet();

                accountFrom.reduce(amo);
                accountTo.add(amo);

                accountTo.reduce(amo);
                accountFrom.add(amo);

                if(accountFrom.getAmount() != 10 || accountTo.getAmount() != 5) {
                    System.out.println(amo + " " + accountFrom + " " + accountTo);
                    break;
                }
         }
    }

W taki rozwiązaniu jest większa szansa, że wątki będą się wykonywać współbieżnie, gdy jeden będzie wykonywał metodę na accountTo to drugi może wykonać coś na accountFrom.

0

A no tak. Nie wiem dlaczego uwidziało mi się, ze nowy wątek wymaga nowego runnabla. Takie coś przechodzi:

public static void main(String[] args) {
		ExecutorService ex = Executors.newFixedThreadPool(5);
		
		Account accountFrom = new Account(10);
		Account accountTo = new Account(5);
		
		AccountTask at = new AccountTask(accountFrom, accountTo);
		ex.execute(at);
		ex.execute(at);
		ex.execute(at);
	}

Natomiast mam jeszcze pytanie jak miałaby wyglądać synchronizacja na współdzielonych obiektach? Takie coś?:

synchronized(accountFrom) {
					synchronized(accountTo) {
                                             //...
					}
}

i czym to się różni od synchronized(this)?

Co do tego uproszczenia to jeśli dobrze je rozumiem to wydaje mi się, że ono nie jest w tym przypadku dobre. Chcę zachować, jakby to powiedzieć, atomiczność 4 operacji. Tzn. mam jakby grupę operacji dodaj do pierwszego konta, odejmij od drugiego, dodaj do drugiego, odejmij od pierwszego. Chciałbym, żeby to wszystko poszło od jednego strzału. Jeśli dam synchronized na metodach add i reduce to one mi się zrobią niezależne od siebie.

Sensowności nie ma co się doszukiwać. Aplikacja ma charakter czysto dydaktyczny.

0

W Twoim kodzie synchronized(this) powoduje założenie locka na obiekcie this, którym jest AccountTask - jeśli tworzysz kilka różnych instancji AccountTask to każda działa na innym locku - nie ma synchronizacji.

Dodawanie synchronized na pojedynczych metodach przy złożonej operacji to tak jak wiara w to, że volatile int x pozwala na coś w tylu x++ - to nie zadziała poprawnie.

Ta konstrukcja rozwiąże problem, tylko przy zagnieżdżonych synchronized trzeba uważać - najlepiej żeby zawsze w kodzie występowały w jednej kolejności, bo inaczej można spowodować deadlock:

synchronized(accountFrom) {
                    synchronized(accountTo) {
                    }
}
1

Pro tip: najlepiej nie synchronizować.
Od synchronizacji lepsze sa współbieżne kolekcje, AtomicReference na niemutowalne obiekty ale pamiętaj że block synchonized może wykorzystywać jakiś obiekt jako locka, ew. możez skorzystać z ReetrantLocka
https://docs.oracle.com/javase/tutorial/essential/concurrency/locksync.html

1 użytkowników online, w tym zalogowanych: 0, gości: 1