(Threads) Wie kann ich negative Werte verhindern?

Hallo Leute,
ich möchte viel üben und habe diese Programmierbeispiel aufgetrieben. in der ausgabe haben doch beide Barmen 6 Bier und warum wird bei Barmen1 doppelt bier genommen?Wo liegt mein Fehler?

In our scenario there are exactly 2 Oktoberfest barmen. These barmen fill a complete
Maß (beer mug) every 2 seconds. Whenever a Maß is filled, the simulator should print
the following text to the console (i.e., usingSystem.out.println):
Barman x: There are currently y Masz waiting to be served. Obviously,x indicates which barman is meant (out of the 2) and y the actual number
of beers that are currently ready to be served to the customers.

There are exactly 15 Oktoberfest waitresses. One waitress is able to carry 6 Maß at once.
It takes her 5 to 10 seconds (random) to deliver one of the Maß she is currently carrying.
She has to get a new set of Maß from the barmen as soon has she has delivered all 6
Maß. It takes her 15 seconds to get back to the bar. Note that the waitress has to wait
for the barmen to draw additional Maß in case there are not at least 6 Maß available.
Whenever a waitress delivered a Maß the simulator should print the following:
Waitress x: Prost! Now I have got y Masz left.
Note that the number
x
is supposed to indicate which waitress (out of the existing 15) is
meant and
y
should reflect the actual number of Maß the waitress is currently carrying.
Additionally, the simulator should print the following whenever a waitress has to get
back to the barman:
Waitress x: I need to get a new set of Mas

Every 5 seconds 4 to 10 new customers (random) are entering the location. Every cus-
tomer orders and drinks exactly 1 Maß and leaves afterwards. Whenever a customer is
served the simulator should print the following text:
Customer: Thanks! I had to wait x seconds to get my Masz from Waitress y.
Note that
x
denotes the amount of seconds that the corresponding customer had to wait
to get the Maß delivered and
y
indicates the waitress who delivered the beer


public class Waitress extends Thread {

	private String name;
	Barman barman1;
	Barman barman2;
	private static final int MIN = 5;
	private static final int MAX = 10;

	public Waitress(String name, Barman barman1, Barman barman2) {
		this.name = name;
		this.barman1 = barman1;
		this.barman2 = barman2;

	}

	public String getWaitressName() {
		return this.name;
	}

	@Override
	public synchronized void run() {
		while (true) {
			while (takeBier() == false) {
				try {
					Thread.sleep(1000);
				} catch (InterruptedException e) {
					// TODO Auto-generated catch block
					e.printStackTrace();
				}
			}

			for (int i = 5; i >= 0; i--) {
				try {
					Thread.sleep(random(MIN, MAX) * 1000);
					Oktoberfest.customers.get(0).setWaitress(this);
					Oktoberfest.customers.remove(0);
					System.out.println("Waitress " + name + " Prost! now I have got " + i + " Masz left");
				} catch (InterruptedException e) {
					// TODO Auto-generated catch block
					e.printStackTrace();
				}

			}
			try {
				System.out.println("Waitress " + name + "I need to get a new set of Masz");
				Thread.sleep(15000);

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

	public boolean takeBier() {
		if (barman1.isEnought() == true) {
			barman1.entleeren();
			return true;
		} else if (barman2.isEnought() == true) {
			barman2.entleeren();
			return true;
		} else
			return false;

	}

	public static int random(int min, int max) {
		return (int) (min + Math.random() * (max - min));
	}
}

public class Barman extends Thread {

	private String name;
	private int anzahl = 0;

	public Barman(String name) {
		this.name = name;

	}

	@Override
	public synchronized void run() {
		while (true) {
			try {
				Thread.sleep(2000);
				fillBier();
			} catch (InterruptedException e) {
				// TODO Auto-generated catch block
				e.printStackTrace();
			}
		}
	}

	public void fillBier() {
		anzahl++;
		System.err.println("Barmen " + name + " there are currently " + anzahl + " Masz waiting to be served");
	}

	public boolean isEnought() {
		if (anzahl >= 6) {
			return true;
		} else {
			return false;
		}
	}

	public void entleeren() {
		anzahl = anzahl - 6;
	}

}

public class Customer extends Thread {
	private String name;
	private long millisEnter = System.currentTimeMillis();
	private Waitress waitress = null;

	@Override
	public synchronized void run() {
		while (waitress == null) {
			try {
				Thread.sleep(1000);
			} catch (InterruptedException e) {
				e.printStackTrace();
			}
		}
		long millisBeerReceived = System.currentTimeMillis();
		System.out.println("Customer: Thanks! I had to wait " + (millisBeerReceived - millisEnter) / 1000
				+ " seconds to get my Masz from Waitress " + waitress.getWaitressName());
	}

	public void setWaitress(Waitress waitress) {
		this.waitress = waitress;
	}
}

import java.util.ArrayList;
import java.util.List;

public class Oktoberfest {

	public static List<Customer> customers = new ArrayList<Customer>();

	public static void main(String[] args) throws InterruptedException {
		synchronized (args) {

			Barman barman1 = new Barman("1");

			Barman barman2 = new Barman("2");

			barman1.start();

			barman2.start();

			List<Waitress> waitres = new ArrayList<Waitress>();
			for (int i = 0; i < 15; i++) {
				Waitress w1 = new Waitress(String.valueOf(i + 1), barman1, barman2);
				waitres.add(w1);
				w1.start();
			}

			while (true) {
				for (int i = 0; i < Waitress.random(4, 10); i++) {
					Customer customer = new Customer();
					customers.add(customer);
					customer.start();
				}
				Thread.sleep(5 * 1000);
			}
		}
	}
}

AUSGABE

Barmen 1 there are currently 5 Masz waiting to be served
Barmen 2 there are currently 6 Masz waiting to be served
Barmen 1 there are currently 6 Masz waiting to be served
Barmen 2 there are currently 1 Masz waiting to be served
Barmen 1 there are currently -5 Masz waiting to be served
Barmen 2 there are currently 2 Masz waiting to be served
Barmen 1 there are currently -4 Masz waiting to be served
Waitress 4 Prost! now I have got 5 Masz left
Customer: Thanks! I had to wait 17 seconds to get my Masz from Waitress 4
Barmen 2 there are currently 3 Masz waiting to be served
Barmen 1 there are currently -3 Masz waiting to be served

Der Fehler liegt darin, dass die “takeBier”-Methode unsynchronisiert von mehreren Threads ausgeführt werden kann. Dabei kann ein Ablauf wie dieser entstehen:


               Thread of Waitress 0          |   Thread of Waitress 1
    ====================================================================================
    public boolean takeBier() {              |
                                             |   public boolean takeBier() {
                                             |       if (barman1.isEnought() == true) {
        if (barman1.isEnought() == true) {   |                
            barman1.entleeren();             |
                                             |           barman1.entleeren();
                                             |           return true; 
                                             |       }
            return true;                     |       ...
        }                                    |   }
        ...                                  |   ...

Beide Threads prüfen erstmal “isEnough”, und das gibt “true” zurück. Aber dann rufen BEIDE “entleeren” auf, wodurch die Zahl negativ wird.

Man könnte jetzt unüberlegt-pragmatisch einen dicken “synchronized”-Block da drumwickeln. Das KÖNNTE funktioneren, und KÖNNTE auch richtig sein (!), aber man sollte sich schon genau überlegen, WO man WIE auf WAS synchronisiert.

Und nebenbei: Es kann sicher (gerade zum Üben) nicht schaden, das man auf dieser Ebene selbst zu machen. Aber oft ist es einfacher (und weeeeit weniger fehleranfällig!), sich auf die existierenden Concurrency-Klassen zu verlassen. Das Beispiel hier ist eine Abwandlung vom https://en.wikipedia.org/wiki/Producer–consumer_problem - sowas kann man oft mit einer https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/BlockingQueue.html lösen. (Auch wenn man sich für die Anforderung, dass immer 6 Biere vorhanden sein müssen, was überlegen müßte. Vielleicht wäre dafür dann ein CountDownLatch oder Phaser das geeignetere)

du bist ein Engel. Danke

synchronized ist ja grundsätzlich schon vorhanden, aber nicht sehr sinnvoll eingebaut:
jeder Thread, also Customer, Waitress, Barmen, synchronisiert seine run-Methode, also seine gesamte Laufzeit

da niemand anders je auf die anderen Akteure synchronisierend zugreift ist das letztlich egal,
wenn doch wäre es im Moment fatal, ein Zugriff gar nicht möglich, dauerhaft blockiert, da run die ganze Zeit läut und den Zugriff hält


takeBier() für sich zu synchronisieren bringt dann nichts, weil wiederum jede Waitress für sich nur ihre eigene Methode aufruft,
immerhin blockieren diese Aufrufe nicht: da die Waitress von run schon den Zugriff hat, darf sie auch takeBier() aufrufen,

es findet jedenfalls keine Synchronisation mit anderen statt, dafür braucht es ein gemeinsames Objekt, auf das alle zugreifen,
hier vielleicht passend ein Objekt Bartheke, grob dort die Methode takeBier() hin verschieben,
synchronized durchaus, alle müssen dort zugreifen

wichtig: nur EINE Bartheke, nicht jeder für sich eine,
aber nur ZWEI Barmen und an die Waitress korrekt übergeben hast du ja auch schon hinbekommen


Übung hier streng zu empfehlen

Mein Hinweis beschrieb die Ursache des Fehlers. Und der erhobene Zeigefinger mit “Da sollte man aufpassen!” kam hoffentlich deutlich genug rüber. Im konkreten Fall könnte man den angedeuteten “dicken synchronized-Block” auf den barman anwenden, im Sinne von

    public boolean takeBier() {
        synchronized(barman1) {
            if (barman1.isEnought() == true) {
                barman1.entleeren();
                return true;
        }
        ...
    }

Denn DAS ist ja das kritische Objekt hier. Natürlich müßte man auch noch schauen, dass es dann keine verändernden, nicht-synchronisierten Zugriffe darauf gibt. Aber ich habe nicht das komplette Programm nachvollzogen

nur so nebenbei weiter aufzählend:
die ganzen catch (InterruptedException e) sollten weg,
schreibe irgendwo eine statische Hilfsmethode warte(int ms) , und rufe die dann jeweils nur auf: Helper.warte(1000); statt mit aufwendigen try/catch


if (barman1.isEnought() == true)

if (barman1.isEnought())

        if (anzahl >= 6) {
            return true;
        } else {
            return false;
        }
    }
->
    public boolean isEnought() {
            return anzahl >= 6;
    }
            barman1.entleeren();
            return true;

könnte zu

            return true;

werden, mit einer Methode in Barman welche true zurückliefert, falls 6 vorhanden sind und diese gleich abzieht,
(auch wenn manche Bedenken haben könnten, wenn eine Methode gleichzeitig etwas ändert + etwas zurückliefert :wink:
https://forum.byte-welt.net/java-forum/allgemeine-themen/20175-seiteneffekt-rueckgabewert.html bloß nicht alles lesen)

diese Methode könnte und sollte dann gleich synchronized sein, Theke ausgeschaltet

takeBeer() könnte zusammenfallen zu

           return barman1.giveSixBeers() || barman2.giveSixBeers();
     }

aber solche starke Verkürzungen sind nicht jedermanns Sache, nur verwenden wenn auch absolutes Verständnis dafür,
für den Anfang ist viel einfacher Code durchaus besser, wobei die boolean if-else nun wirklich nicht sein müssten