Problem mit Synchronisierung

[_Andi91]

Hi,

Ich habe einen ObjectPool geschrieben. Dieser soll threadsafe sein, wenn ich im Konstruktor true übergebe.
Intern wird dann ein Flag gesetzt (concurrent).

Wenn ich von "außen" ein Objekt aus dem Pool haben möchte, rufe ich die Methode get() auf.
Die Methode ruft dann wenn concurrent true ist, eine Methode getSync() auf, diese ist synchronisiert und ruft wieder die Methode getIntern() auf.
Ist concurrent false, wird direkt die Methode getIntern() aufgerufen.

Code:

public T get()
{
	//Wenn concurrent -> synchronisiert, sonst normal
	if(concurrent)
	{
		return getSync();
	}
	return getIntern();
}

private synchronized T getSync()
{
	return getIntern();
}

private T getIntern()
{
	....
}

Ich hab das ganze jetzt mal getestet, in dem ich mehrere Threads auf einen Pool gleichzeitig die get() Methode aufrufen lasse.

Das Limit für die maximale Anzahl an Elementen ist 50.
Nach die Threads gestartet sind (1000 Threads) lasse ich mir die Anzahl der Objekte im Pool zurück geben. Teilweise kommt dabei sogar sowas wie 390 raus :autsch:

An der Implementierung der getIntern() Methode liegt es nicht. Wenn ich die get() Methode synchronisiert mache, hab ich am Ende immer maximal 50 Instanzen im Pool egal wie viel Threads ich parallel starte.

Ich habe auch mal in der get() Methode vor der Zeile

return getIntern();

eine sysout Ausgabe gemacht. Komischerweise wird diese tatsächlich ein paar Mal angezeigt. Wie kann das sein? Es handelt sich immer um das gleiche pool Objekt und da ist das Flag concurrent auf true gesetzt worden im Konstruktor???:L


mfg, Andi

 

[SlaterB]

deine Erläuterungen sind ziemlich nichtssagend (du beschreibst dein Problem ja, aber alles zu 'müsste doch so und so sein' ist nicht begründet/ nachprüfbar),
dein Code beliebig, hängt alles vom Rest ab,

du müsstest nur ein lauffähiges Programm posten und schon könnte alles zu 100% exakt getestet werden, fertig

 

[_Andi91]

mh ok, dachte es ist vielleicht auch so verständlich genug.
Hab die Sourcen angehängt

EDIT: Die Methode getIntern() in der klasse ObjectPool ist eigentlich nicht synchronized. Ist aus Testzwecken falsch in der Klasse im Anhang

 

[_Andi91]

Oh mann bin ich blöd^^
Hab vergessen die Membervariable concurrent zu setzen
Jetzt gehts.

 

[SlaterB]

ziemlich frech, zum Glück ein Thema der interessanteren Sorte,
bei mir kommt es nicht zu 390, nur selten mal zu 51 oder 52 statt 50,

ich denke es liegt am hinteren Teil von getIntern():

 // kein freies object gefunden
        if (obj == null)
        {
            // Noch platz im Pool -> neues Object erzeugen und dem Pool hinzufuegen
            if (poolObjects.size() < poolMethodsImpl.getMaxSize())
            {
                obj = new PoolObject<T>(poolMethodsImpl.getNewObj(), true);
                poolObjects.add(obj);
            }

dort werden neue Objekte erzeugt, ohne Synchronisation können bei 49 vorhandenen Objekten gerade mehrere Threads das if mit der size-Prüfung passieren, bevor der erste von diesen das 50. Objekt erzeugt
-> die anderen Threads fügen auch noch ein, die maximale Size wird überschritten,

besonders deutlich wird es, wenn du Thread.sleep(500); in das if vor dem add() einbaust, durch die lange Wartezeit schaffen es alle Threads in das if hinein, am Ende ist die size 1000,

in der main-Methode solltest du auch eine gewisse Wartezeit vor der Ausgabe der size() einbauen, sonst kann dort ein Stand ausgegeben werden, bevor überhaupt alle Threads durch sind,
Ausgabe 0 habe ich teilweise auch

 

[_Andi91]

ich denke es liegt am hinteren Teil von getIntern()
dort werden neue Objekte erzeugt, ohne Synchronisation können bei 49 vorhandenen Objekten gerade mehrere Threads das if mit der size-Prüfung passieren, bevor der erste von diesen das 50. Objekt erzeugt
-> die anderen Threads fügen auch noch ein, die maximale Size wird überschritten,

besonders deutlich wird es, wenn du Thread.sleep(500); in das if vor dem add() einbaust, durch die lange Wartezeit schaffen es alle Threads in das if hinein, am Ende ist die size 1000,

Stimmt zwar aber im Prinzip aber der Block bzw. die gesamte getIntern() Methode ist schon synchronisiert. Vorraussetzung ist halt, dass sie von der getSync invoked wird und nicht direkt über get().
Aber so war es, weil concurrent nicht gesetzt wurde und somit immer die Methode getIntern() direkt über get() ohne synchronisation invoked wurde.

 

[SlaterB]

das war doch auch dein Ziel oder? dass man getIntern() ohne Synchronisation ausführen kann?
wenn concurrent immer an ist, brauchst du gar nicht erst das Klassenattribut/ die Unterscheidung

wenn es aber irgendeine Möglichkeit gibt, getIntern() ohne Synchronisation ausführen, dann hast du eben durch das Einfügen ein Problem,
ist dir vielleicht auch klar, nur dann ist die ursprüngliche Frage, die das Hinzufügen im getIntern() überhaupt nicht thematisiert hat, etwas merkwürdig, was wolltest du überhaupt wissen?

 

[_Andi91]

Glaub wir reden grad bissl aneinander vorbei

Hast es schon richtig verstanden.
Aber bei dem Konstruktor Aufruf mit dem boolean hab ich diese Zeile vergessen
[XML]this.concurrent = concurrent;[/XML]

Deswegen war concurrent immer auf false und deswegen kam es zu dem Problem, dass zu viele Instanzen hinzugefuegt werden können, weil die getIntern() nie synchronisiert aufgerufen wurde.

 

[Kr0e]

Ich wuerde es schoener und globaler machen: Erwarte als Parameter eine Instanz vom Typ Lock. Wenn nich synchronisiert wird, uebergibts du ein Dummy-Lock ohne Funktion. SO kannst du alle Klassen die du schreibst immer grundsaetzlich synchronisiert entwickeln und wenn dann mal Sync angebracht ist, nimm einfach ein Reentrantlock und zack ! Flexibler gehts nicht

 

[_Andi91]

Mh ob das so schöner wäre?
für den Benutzer der Klasse macht es ja keinen grossen Unterschied, ob ich jetzt ein ReentrantLock im Konstruktor übergeben muss oder einen boolean. Ich bräuchte halt keine drei Methoden mehr und müsste nur noch in der get() am Anfang lock und am Ende unlock machen.
Dafür bräuchte man aber noch die Dummy Implementierung von Lock.
Also ich finde schöner ist des auch nicht unbedingt.

Was mir aber besser gefällt, ist weiterhin die Übergabe eines boolean und dass dann je nachdem intern entweder ein Dummy Lock oder ein ReentrantLock gesetzt wird.

 

[Kr0e]

Es ist auf jeden Fall flexibler, wenn du die boolean-Uebergabe os magst, dann kannst du noch nen dritten Konstr. machen.
Das beste ist, dass deine Klassen dadurch super strukturiert werden. Hast du hinterher mal die Notwendigkeit, dass dein Object einen existieren Lock nutzt von einer Liste zB. dann kannste einfach deren Lock nehmen. Aber diese if boolean Technik ist natuerlich ausreichend. Wollte das nur mal als kleinen Gedankenansatz in den Ring werfen fuer allgemeine Programmoptimierung...