Archief - [JAVA] concurrency

Het archief is een bevroren moment uit een vorige versie van dit forum, met andere regels en andere bazen. Deze posts weerspiegelen op geen enkele manier onze huidige ideeën, waarden of wereldbeelden en zijn op sommige plaatsen gecensureerd wegens ontoelaatbaar. Veel zijn in een andere tijdsgeest gemaakt, al dan niet ironisch - zoals in het ironische subforum Off-Topic - en zouden op dit moment niet meer gepost (mogen) worden. Toch bieden we dit archief nog graag aan als informatiedatabank en naslagwerk. Lees er hier meer over of start een gesprek met anderen.

forloRn_

Legacy Member
Hey. Ik heb het onderstaande ook op javaranch.com gepost, vandaar het Engels. 't Is een voorbeeldje uit Thinking in Java van Bruce Eckel uit het hoofdstuk Concurrency. 't Is op zich niet moeilijk, maar ik denk dat er een fout in zit. Kan iemand dat eens bevestigen?

Hello,

Below is an example in Bruce Eckel's book Thinking in Java. Don't let the number of lines scare you away, it's actually pretty straightforward. For improved readability, you can just paste it in your IDE if you want to, it's a single file with no dependencies. I've added a few comments here and there for my (and your) convenience.

What the program does is simulate a line of customers queueing in front of a number of teller machines. Each customer is served by a teller for a certain amount of time. Additional customers get in line while the programming is running, and the number of active tellers adjusts itself to the total number of customers (in the TellerManager class).

The compareTo() method in Teller is used by the PriorityQueue in TellerManager and is used for sorting the Tellers in said queue, for example upon calling workingTellers.add(teller). These add methods are called in TellerManager.adjustTellerNumber(), which effectively runs in a dedicated TellerManager thread. You can see that in TellerManager's run() method.

Now, Bruce Eckel has synchronized the Teller.compareTo() method. Since Teller.customersServed is shared between the Teller's thread (look at its run() method) and the TellerManager's thread (which calls workingTellers.add(teller), which calls Teller.compareTo(Teller)), I can see why synchronization is necessary.

The part I'm having a problem with is the implementation of compareTo() on line 106. It makes use of other.customersServed. When TellerManager takes the lock on this, it makes sure that this.customersServed cannot be changed while the lock is held, but in the meanwhile, other.customersServed can still be changed by an other thread, since this thread isn't locking on other. What do you think, am I right?

I'd solve the problem with a private synchronized getCustomersServed() method in Teller, and call this method instead of accessing the field directly.

Code:
import java.util.concurrent.*;
import java.util.*;

// Read-only objects don’t require synchronization:
class Customer {
	private final int serviceTime; // duration of this customer's actions

	public Customer(int tm) {
		serviceTime = tm;
	}

	public int getServiceTime() {
		return serviceTime;
	}

	@Override public String toString() {
		return "[" + serviceTime + "]";
	}
}

// Teach the customer line to display itself:
class CustomerLine extends ArrayBlockingQueue<Customer> {

	public CustomerLine(int maxLineSize) {
		super(maxLineSize);
	}

	@Override public String toString() {
		if(this.size() == 0)
			return "[Empty]";
		StringBuilder result = new StringBuilder();
		for(Customer customer : this)
			result.append(customer);  // display customers
		return result.toString();
	}
}

// Randomly add customers to a queue:
class CustomerGenerator implements Runnable {

	private CustomerLine customers;
	private static Random rand = new Random(47);

	public CustomerGenerator(CustomerLine cq) {
		customers = cq;
	}

	@Override public void run() {
		try {
			while(!Thread.interrupted()) {
				TimeUnit.MILLISECONDS.sleep(rand.nextInt(300)); // add new customer every 150 ms
				customers.put(new Customer(rand.nextInt(1000))); // his actions take 500 ms
			}
		} catch(InterruptedException e) {
			System.out.println("CustomerGenerator interrupted");
		}
		System.out.println("CustomerGenerator terminating");
	}
}

class Teller implements Runnable, Comparable<Teller> {
	private static int counter = 0; // id
	private final int id = counter++;

	// Customers served during this shift:
	private int customersServed = 0; // count how much served
	private CustomerLine customers;
	private boolean servingCustomerLine = true;

	public Teller(CustomerLine cq) {
		customers = cq;
	}

	@Override public void run() {
		try {
			while(!Thread.interrupted()) {
				Customer customer = customers.take(); // keep serving new customers
				TimeUnit.MILLISECONDS.sleep(customer.getServiceTime());

				synchronized(this) {
					customersServed++;
					while(!servingCustomerLine) // disable
						wait();
				}
			}
		} catch(InterruptedException e) {
			System.out.println(this + "interrupted");
		}
		System.out.println(this + "terminating");
	}

	public synchronized void doSomethingElse() {
		customersServed = 0;
		servingCustomerLine = false;
	}

	public synchronized void serveCustomerLine() {
		assert !servingCustomerLine:"already serving: " + this;
		servingCustomerLine = true;
		notifyAll(); // wake up serving thread
	}

	@Override public String toString() { return "Teller " + id + " "; }

	// Used by priority queue:
	public synchronized int compareTo(Teller other) {
		return customersServed < other.customersServed ? -1 :
	(customersServed == other.customersServed ? 0 : 1);
	}

	public String shortString() { return "T" + id; }
}

class TellerManager implements Runnable {
	private ExecutorService exec;
	private CustomerLine customers;

	private PriorityQueue<Teller> workingTellers = new PriorityQueue<Teller>(); // non-blocking
	private Queue<Teller> tellersDoingOtherThings = new LinkedList<Teller>();

	private int adjustmentPeriod;
	
	public TellerManager(ExecutorService e,
			CustomerLine customers, int adjustmentPeriod) {
		exec = e;
		this.customers = customers;
		this.adjustmentPeriod = adjustmentPeriod;

		// Start with a single teller:
		Teller teller = new Teller(customers);
		exec.execute(teller);
		workingTellers.add(teller);
	}

	private void adjustTellerNumber() {
		// This is actually a control system. By adjusting
		// the numbers, you can reveal stability issues in
		// the control mechanism.

		// If line is too long, add another teller:
		if(customers.size() / workingTellers.size() > 2) {
			// If tellers are on break or doing
			// another job, bring one back:
			if(tellersDoingOtherThings.size() > 0) {
				Teller teller = tellersDoingOtherThings.remove();
				teller.serveCustomerLine();
				workingTellers.add(teller);
			} else {
				// else create (hire) a new teller
				Teller teller = new Teller(customers);
				exec.execute(teller);
				workingTellers.add(teller);
			}
			return;
		}

		// If line is short enough, remove a teller:
		if(workingTellers.size() > 1 // always keep one teller
				&& customers.size() / workingTellers.size() < 2)
			reassignOneTeller();

		// If there is no line, we only need one teller:
		if(customers.size() == 0)
			while(workingTellers.size() > 1)
				reassignOneTeller();
	}
	
	// Give a teller a different job or a break:
	private void reassignOneTeller() {
		Teller teller = workingTellers.poll(); // remove
		teller.doSomethingElse();
		tellersDoingOtherThings.offer(teller);
	}
	
	@Override public void run() {
		try {
			while(!Thread.interrupted()) {
				System.out.print(customers + " { ");
				for(Teller teller : workingTellers)
					System.out.print(teller.shortString() + " ");
				System.out.println("}");

				TimeUnit.MILLISECONDS.sleep(adjustmentPeriod);
				adjustTellerNumber(); // add, remove tellers according to number of customers
			}
		} catch(InterruptedException e) {
			System.out.println(this + "interrupted");
		}
		System.out.println(this + "terminating");
	}
	public String toString() { return "TellerManager "; }
}

public class BankTellerSimulation {
	static final int MAX_LINE_SIZE = 50;
	static final int ADJUSTMENT_PERIOD = 1000;
	public static void main(String[] args) throws Exception {
		ExecutorService exec = Executors.newCachedThreadPool();
		// If line is too long, customers will leave:
		CustomerLine customers =
			new CustomerLine(MAX_LINE_SIZE);
		exec.execute(new CustomerGenerator(customers));
		// Manager will add and remove tellers as necessary:
		exec.execute(new TellerManager(
				exec, customers, ADJUSTMENT_PERIOD));
		if(args.length > 0) // Optional argument
			TimeUnit.SECONDS.sleep(new Integer(args[0]));
		else {
			System.out.println("Press ‘Enter’ to quit");
			System.in.read();
		}
		exec.shutdownNow();
	}
}

Ollie

Legacy Member
Dat hangt af van wat Eckel wou bereiken (de comparator houdt op het eerste zicht geen steek). Als het de bedoeling was een willekeurige Teller inactive te maken (reassign one teller) dan werkt de code mijns inziens zoals het hoort.

Ik kan er natuurlijk ook helemaal naast zitten (nogal waarschijnlijk, concurrency is geen specialiteit van mij)

forloRn_

Legacy Member
Ja, het nut van die comparator, daar heb ik me ook al het hoofd over zitten breken.

Bruce Eckel zei:
To choose the next teller to put back on the line, the compareTo( ) method looks at the number of customers served so that a PriorityQueue can automatically put the least-worked teller at the forefront.

"to put back on the line", is dat klanten gaan bedienen of idle worden? Als het "klanten gaan bedienen" betekent, dan slaat het op niets, want je hebt de verse Teller al uit de gewone Queue (tellersDoingOtherThings) gehaald:
Code:
Teller teller = tellersDoingOtherThings.remove();
				teller.serveCustomerLine();
				workingTellers.add(teller);
En als het "idle worden" betekent, slaat het ook op niks, want PriorityQueue sorteert wanneer je er elementen in stopt en niet wanneer je ze eruit haalt.
Code:
	private void reassignOneTeller() {
		Teller teller = workingTellers.poll(); // remove
		teller.doSomethingElse();
		tellersDoingOtherThings.offer(teller);
	}
:confused:

Nu, het nut van die comparator is naast de kwestie. Als de TellerManager-thread lockt op teller1, en die thread roept teller1.compare(Teller other) aan, dan is teller1.customersServed veilig, maar other.customersServed volgens mij niet.

Ollie

Legacy Member
forloRn_ zei:
"to put back on the line", is dat klanten gaan bedienen of idle worden? Als het "klanten gaan bedienen" betekent, dan slaat het op niets, want je hebt de verse Teller al uit de gewone Queue (tellersDoingOtherThings) gehaald:
Code:
Teller teller = tellersDoingOtherThings.remove();
				teller.serveCustomerLine();
				workingTellers.add(teller);

Ik versta daaronder "terug actief worden". De add method van de queue zal uiteindelijk de compareTo method oproepen die de nieuwe Teller aan de head zal inserten (maar waarom dan niet hardcoded return -1, waarom de customersServed vergelijken als je weet dat een nieuwe of gereactiveerde Teller 0 customersServed heeft en dus normaal gezien nooit meer klanten zal bediend hebben dan de other teller waarmee vergeleken wordt?).

forloRn_ zei:
Nu, het nut van die comparator is naast de kwestie. Als de TellerManager-thread lockt op teller1, en die thread roept teller1.compare(Teller other) aan, dan is teller1.customersServed veilig, maar other.customersServed volgens mij niet.

Volgens mij ook niet maar als je de nieuwe Teller toch vooraan insert dan maakt het niet zoveel uit.

forloRn_

Legacy Member
Ollie zei:
Ik versta daaronder "terug actief worden". De add method van de queue zal uiteindelijk de compareTo method oproepen die de nieuwe Teller aan de head zal inserten (maar waarom dan niet hardcoded return -1, waarom de customersServed vergelijken als je weet dat een nieuwe of gereactiveerde Teller 0 customersServed heeft en dus normaal gezien nooit meer klanten zal bediend hebben dan de other teller waarmee vergeleken wordt?).

Dacht ik eerst ook, maar teller.serveCustomerLine() doet de wachtende thread in Teller.run() opnieuw lopen. Tussen de volgende twee regels kan die thread dus teller.customersServed al beginnen verhogen. Maar het nut ontgaat me nog steeds.

Code:
teller.serveCustomerLine();
workingTellers.add(teller);

Ollie zei:
Volgens mij ook niet maar als je de nieuwe Teller toch vooraan insert dan maakt het niet zoveel uit.
Neenee, ik bedoel dat als je níet synchroniseert op other, 1) de thread niet noodzakelijk het veranderen van other.customersServed zal zien, en 2) verschillende waarden kan zien van other.customersServed binnen die methode:

Code:
public synchronized int compareTo(Teller other) {
		return customersServed < other.customersServed ? -1 :
	(customersServed == other.customersServed ? 0 : 1);
	}

Je ziet daar twee leesoperaties van other.customersServed staan, juist? Tijdens het doorlopen van deze methode zit een andere thread (die other.run() uitvoert) other.customersServed te wijzigen dus kan je hier twee verschillende waarden lezen.

Ollie

Legacy Member
forloRn_ zei:
Dacht ik eerst ook, maar teller.serveCustomerLine() doet de wachtende thread in Teller.run() opnieuw lopen. Tussen de volgende twee regels kan die thread dus teller.customersServed al beginnen verhogen. Maar het nut ontgaat me nog steeds.

Theoretisch mogelijk inderdaad (ook met nieuwe Tellers trouwens), maar mijn testen met verschillende sleep waarden gaven altijd hetzelfde resultaat (ie nieuwe Teller vooraan de queue en als eerste terug inactief). Niet dat dat iets verandert aan het feit dat zijn access niet gesynchronizeerd is natuurlijk.

Is het trouwens gegarandeerd dat beide other.customersServed's op beide lijnen dezelfde waarde hebben in de code hieronder?

Code:
public synchronized int compareTo(Teller other) {
	return customersServed < other.customersServed ? -1 :
	    (customersServed == other.customersServed ? 0 : 1);
}

forloRn_

Legacy Member
Ollie zei:
Is het trouwens gegarandeerd dat beide other.customersServed's op beide lijnen dezelfde waarde hebben in de code hieronder?

Code:
public synchronized int compareTo(Teller other) {
	return customersServed < other.customersServed ? -1 :
	    (customersServed == other.customersServed ? 0 : 1);
}

Nee, volgens mij dus niet (zie mijn vorige berichtje).

Gurdt

Legacy Member
synchronized wil zeggen: een gesynchornized method vn een object (classe, vaak afgeleid vn een thread) kan slechts 1 keer opgeroepen worden tegelijk

dus als ge 2 tellers hebt (kheb niet alles gelezen) zou ge normaal slechts 1 keer de functie compareTo(...) tegelijk kunnen oproepen, als een andere da ook wilt gaat die ff blocken
--
dus volgens mij kunt ge dat wel garanderen (hangt natuurlijk ook van uw andere functies af)
wat natuurlijk ook het hele punt van synchronized is.

forloRn_

Legacy Member
Daar gaat het niet om. Er is maar één thread die compareTo() aanroept, en dat is de thread die TellerManager().run() uitvoert. Die thread neemt de lock op een Teller-object door het aanroepen van compareTo(), omdat compareTo() synchronized is. Dit heeft als gevolg dat Teller.run() blockt op synchronized (this) { ... }. Tot daar alles okee.

Het probleem is dat other.customersServed nog altijd gewijzigd kan worden door een andere thread, aangezien die andere thread de lock op other neemt en niet op dít object.

Locken gebeurt nog altijd op objectniveau, en niet op klasseniveau. Dat kan je ook zien aan synchronized (this).

forloRn_

Legacy Member
Voilà, die compareTo() kan nooit werken. Als je hem vervangt door deze (functioneel identieke), en het bereik van de random-waarden wat verkleint, krijg je een mooie foutboodschap. Het crasht sowieso, maar Thread.yield() doet het wat sneller crashen.

Code:
	public synchronized int compareTo(Teller other) {		

	int s1 = other.customersServed;
	Thread.yield();
	int s2 = other.customersServed;
						
	if (s1 != s2) {
		System.err.println("oh fiddlesticks: s1 == " + s1 + ", s2 == " + s2);
		System.exit(1);
	}
		
		return customersServed < s1 ? -1 :
			(customersServed == s2 ? 0 : 1);
	}
Het archief is een bevroren moment uit een vorige versie van dit forum, met andere regels en andere bazen. Deze posts weerspiegelen op geen enkele manier onze huidige ideeën, waarden of wereldbeelden en zijn op sommige plaatsen gecensureerd wegens ontoelaatbaar. Veel zijn in een andere tijdsgeest gemaakt, al dan niet ironisch - zoals in het ironische subforum Off-Topic - en zouden op dit moment niet meer gepost (mogen) worden. Toch bieden we dit archief nog graag aan als informatiedatabank en naslagwerk. Lees er hier meer over of start een gesprek met anderen.
Terug
Bovenaan