ArrayList.size() zwraca błędny wynik.

Witam ponownie,

Mam kłopot z poprawną obsługą ArrayListy przez wiele wątków. Gdy któryś wątek usuwa element z listy, nie zawsze po tej operacji otrzymuję poprawny rozmiar listy po wywołaniu metody .size(). Raz jest dobrze, raz źle, ogólne randomowo to trochę działa. Po usunięciu danego elementu sprawdzam metodą .contains() czy obiekt faktycznie został usunięty i za każdym razem tak się dzieje. Czy ktoś mógłby mi wytłumaczyć co robię źle? Idea programu jest taka, że kolejne wątki klientów próbują się dostać do muzeum, które ma określoną pojemność.

Klasa Muzeum:

import java.util.ArrayList;

public class Muzeum {
	
	private ArrayList<Klient> clients;
	public int size=0;
	
	public Muzeum(int size) {
		this.size=size;
		clients=new ArrayList<Klient>(size);
		clients.trimToSize();
	}
	
	public synchronized void enterMuseum (Klient k) {
		if(clients.size()<size) {
			clients.add(k);
			System.out.println(clients.contains(k));
			System.out.println("Client: "+k.clientID + " entered. INSIDE: " + howManyInside());
		}
		else
		{
			System.out.println("Room is full");
		}
	}
	
	public synchronized void leaveMuseum(Klient k) {
		clients.remove(k);
		System.out.println(clients.contains(k));
		System.out.println("Client: "+ k.clientID+ " left. INSIDE: " + howManyInside());
	}
	
	public int howManyInside () {
		return clients.size();
	}

}

Klasa Klient:

public class Klient implements Runnable {

	public int clientID;
	public Muzeum muzeum;
	public long time;
	
	public Klient(int id, Muzeum muzeum) {
		clientID=id;
		this.muzeum=muzeum;
	}
	@Override
	public void run() {
		// TODO Auto-generated method stub
		while(true) {
			try {
				
				time=(long)(Math.random()*15);
				Thread.sleep(time*1000);
			} catch(Exception e) {
				
			}
			
			muzeum.enterMuseum(this);
			try {
				
				time=(long)(Math.random()*100);
				Thread.sleep(time*1000);
			} catch (Exception e) {
				
			}
			
			muzeum.leaveMuseum(this);
			
		
		}
	}

}

Odpalenie w mainie:

 public class Test {

	/**
	 * @param args
	 */
	public static void main(String[] args) {
		// TODO Auto-generated method stub
		
		Muzeum muzeum=new Muzeum(2);
		
		
		for(int i=0;i<4;i++) {
			new Thread(new Klient(i, muzeum)).start();
		}
		
		
	}

}

dodanie znaczników <code class="java"> - Furious Programming

Spróbuj zastąpić funkcję

howManyInside()

przez wywołanie bezpośrednio clients.size()

.
Coś w tym stylu:
```java
System.out.println("Client: "+ k.clientID+ " left. INSIDE: " + clients.size());

bo chyba o to Ci chodzi.

Tak robiłem na początku i było to samo :)

@Endrew Czy przypadkiem nie jest tak, że np. wchodzi 1 i 2 a mimo to może wyjść np 3 który wcale tam nie wchodził i wtedy w środku nadal jest dwóch? Sprawdź to. :)
edit:
Coś takiego w funkcji do wychodzenia powinno pomóc:

if(clients.contains(k)) {
  	clients.remove(k);
        System.out.println(clients.contains(k));
        System.out.println("Client: "+ k.clientID+ " left. INSIDE: " + howManyInside());
}

Faktycznie, po zmianie na:

public synchronized void leaveMuseum(Klient k) {
		if(clients.contains(k)) {
			clients.remove(k);
			System.out.println("Client: "+ k.clientID+ " left. INSIDE: " + howManyInside());
		} 

wygląda na to, że liczy już dobrze, ale muszę jeszcze trochę potestować :)

dodanie znacznika <code class="java"> - Furious Programming

Widziałem, widziałem :)
Chyba już wszystko dobrze hula :) Dzięki @szweszwe :)

Fragment dokumentacji klasy ArrayList:

Note that this implementation is not synchronized. If multiple threads access an ArrayList instance concurrently, and at least one of the threads modifies the list structurally, it must be synchronized externally. (A structural modification is any operation that adds or deletes one or more elements, or explicitly resizes the backing array; merely setting the value of an element is not a structural modification.) This is typically accomplished by synchronizing on some object that naturally encapsulates the list. If no such object exists, the list should be "wrapped" using the Collections.synchronizedList method. This is best done at creation time, to prevent accidental unsynchronized access to the list:

List list = Collections.synchronizedList(new ArrayList(...));

Czasami warto zajrzeć do dokumentacji :P

@bogdans
Dopytam, bo nie do końca rozumiem :)

Czyli nawet jeśli metody dostępowe do ArrayListy są synchronized to i tak muszę jeszcze synchronizować samą ArrayListę przy użyciu podanej przez Ciebie metody? Troszkę mi się to dziwne wydaje, dlatego się dopytuję :)

Dwie sprawy.

1. Ty chyba błędnie interpretujesz komunikaty wypisywane przez program, a te komunikaty są nieprawdziwe.

    public void leaveMuseum(Klient k) {
        clients.remove(k);
        System.out.println(clients.contains(k));
        System.out.println("Client: "+ k.clientID+ " left. INSIDE: " + howManyInside());
    }

Jeżeli metoda enterMuseum(klient) została wywołana w momencie gdy muzeum było pełne, to ten klient nie wszedł do muzeum, zatem kolejne wywołanie leaveMuseum(klient) wyświetli nieprawdziwy komunikat o opuszczeniu muzeum. Metoda powinna wyglądać raczej tak:

    public void leaveMuseum(Klient k) {
        if(clients.remove(k))
            System.out.println("Client: "+ k.clientID+ " left. INSIDE: " + howManyInside());
    }

Po tej poprawce nie dostrzegłem ani razu niepoprawnych informacji o ilości klientów w muzeum (mimo braku jakiejkolwiek synchronizacji).
2. Konstruowanie ArrayList w taki sposób:

List list = Collections.synchronizedList(new ArrayList(...));

powoduje, że metody zmieniające liczebność ArrayList (dodawanie i usuwanie elementów) nie muszą być synchronizowane. Inne rozwiązanie, to użycie klasy Vector zamiast ArrayList.

Ok, teraz już chyba wszystko jasne.

Dzięki za wyjaśnienie :)