[Java] Synchronisation: ist das korrekt gecodet?

[Captain America]

Hi there, ich habe hier ein Codesnippet (gekürzt), von dem ich mir nicht so ganz sicher bin, ob die Synchronisation korrekt ist. Diese Klasse ist Teil eines Client und Server Programms, welches es erlauben soll, mehreren Threads über die Methode write() zu senden. Die Ausgabe erfolgt gepuffert (Objekt buffer), falls es Lags in der Leitung gibt. Ist das richtig synchronisiert? Das kommt mir seltsam vor.


import java.io.*;
import java.net.*;
import java.util.*;



public class Watdatn extends Thread
{
ObjectOutputStream oos;
boolean run;
Queue<String> buffer;
final static int MAXQUEUE = 10;


public Watdatn(ObjectOutputStream oos)
{
super();
this.oos = oos;

buffer = new LinkedList<String>();

run = true;
}



public void write(String s)
{
try
{
synchronized(buffer)
{
while(buffer.size() == MAXQUEUE)
{
buffer.wait();
}

buffer.offer(s);
buffer.notify();
}
}
catch(InterruptedException e)
{
e.printStackTrace();
}
}



public void run()
{
String s = null;

while(run)
{
try
{
synchronized(buffer)
{
while(buffer.size() == 0)
{
buffer.wait();
}

s = buffer.poll();
buffer.notify();
}

oos.writeObject(s);
oos.flush();
s = null;
}
catch(IOException e)
{
e.printStackTrace();
}
catch(InterruptedException e)
{
e.printStackTrace();
}
}
}
}


Bitte nicht lachen, um 6 Uhr morgens gehackt. :ugly:

[Monger]

Hi there, ich habe hier ein Codesnippet (gekürzt), von dem ich mir nicht so ganz sicher bin, ob die Synchronisation korrekt ist. Diese Klasse ist Teil eines Client und Server Programms, welches es erlauben soll, mehreren Threads über die Methode write() zu senden. Die Ausgabe erfolgt gepuffert (Objekt buffer), falls es Lags in der Leitung gibt. Ist das richtig synchronisiert? Das kommt mir seltsam vor.
...


Hrm...

Hab keine Erfahrung mit Thread Programmierung, aber...

Sobald jemand auf deine write() Methode zugreift, ist der Zugriff auf buffer imo komplett gesperrt. Ist das gewollt? Oder willst du nur den Schreibzugriff synchronisieren?

Das normale Pattern sieht ja so aus:


Object mutex = new Object();

public bleh(){

synchronized(mutex){
// hier dein Zugriff auf buffer rein
}
}

Wie gesagt, so wie du es machst sperrst du afaik komplett den Zugriff auf buffer, auch von anderen Methoden her. Kann aber auch sein dass ich Dummfug labere, ich bitte um freundliche Korrektur...

Edit:
Deine Try...Catch Geschichten scheinen mir aber ein bißchen wild zu sein ;)
Der Sinn einer Semaphore sollte doch gerade sein, dass das Programm dort wartet bis es weitermachen darf, und nicht einfach überspringt.

Nochn Edit:
Blöde Korrektur wieder entfernt. Ich sollte echt schlafen gehen und die Klappe halten...

[HellHorse]

Sobald jemand auf deine write() Methode zugreift, ist der Zugriff auf buffer imo komplett gesperrt. Ist das gewollt? Oder willst du nur den Schreibzugriff synchronisieren?
Ich denke schon, dass das so gewollt ist, schliesslich ist die Datenstruktur ja nicht thread-safe, hat eine begrenzte Grösse und was soll denn der Schreibthread machen solange der Puffer leer ist? Zudem gibt wait() den Monitor wieder frei.
Das Problem ist aber dass notify() bloss einen wartenden Thread benachrichtigt. So kann es sein, dass man von einem Schreibthread aus einen anderen Schreibthread aufweckt. Das ist natürlich nicht Sinn und Zweck der Übung. Darum notifyAll() verwenden. Sowieso ist die Regel immer notifyAll() zu verwenden ausser man ist sich verdammt sicher, dass notify() auch reicht.

Edit:
Deine Try...Catch Geschichten scheinen mir aber ein bißchen wild zu sein ;)

Ja, bei einer InterruptedException will man die Threads wohl beenden gleich wie bei einer IOException.

Der Sinn einer Semaphore sollte doch gerade sein, dass das Programm dort wartet bis es weitermachen darf, und nicht einfach überspringt.
Macht er ja auch.

Jetzt kommt aber der wichtige Teil:
Da CapM Java 1.5 verweden darf ist das alles schon implementiert:
http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/BlockingQueue.html

[Captain America]

Hrm...

Hab keine Erfahrung mit Thread Programmierung, aber...

Sobald jemand auf deine write() Methode zugreift, ist der Zugriff auf buffer imo komplett gesperrt. Ist das gewollt? Oder willst du nur den Schreibzugriff synchronisieren?



Das ist so gewollt, denn während man in den Buffer schreibt, darf man nicht aus ihm lesen, und während man aus ihm liesst, nicht in ihn schreiben. Die offer() und poll() Methoden sind auch ziemlich kurz. Eigentlich sind sie so kurz, dass man das ganze nicht syncen braucht. :ugly:



Das normale Pattern sieht ja so aus:


Object mutex = new Object();

public bleh(){

synchronized(mutex){
// hier dein Zugriff auf buffer rein
}
}


Wie gesagt, so wie du es machst sperrst du afaik komplett den Zugriff auf buffer, auch von anderen Methoden her. Kann aber auch sein dass ich Dummfug labere, ich bitte um freundliche Korrektur...



Ich glaube nicht, weil der Thread (Watdatn) selbst der einzige ist, der sich wait()ed und notify()ed. Aber sicher bin ich mir da nicht.

Was mir Sorgen bereitet: wenn ich buffer in einem synchronized() Block habe, und innerhalb dessen wait() aufrufe, kann es dann überhaupt sein, dass ich an einer anderen Stelle die ebenfalls mit buffer synchronized() mit notify[All]() wecke?

Oder meintest du in der obigen Passage das selbe wie ich? ;)

Mal eben Testhacken... :ugly2:


Edit:
Deine Try...Catch Geschichten scheinen mir aber ein bißchen wild zu sein ;)
Der Sinn einer Semaphore sollte doch gerade sein, dass das Programm dort wartet bis es weitermachen darf, und nicht einfach überspringt.

Nochn Edit:
Blöde Korrektur wieder entfernt. Ich sollte echt schlafen gehen und die Klappe halten...

try catch muss tatsächlich so gesetzt werden. denn sobald eine InterruptedException aufgefangen wird, bedeutet das, dass dein Programm sich selbst beenden soll oder wird.

[HellHorse]

Ich glaube nicht, weil der Thread (Watdatn) selbst der einzige ist, der sich wait()ed und notify()ed. Aber sicher bin ich mir da nicht.
Moment, moment, write(String) wird doch sicher von einem anderen Thread aus aufgerufen, oder nicht?

Was mir Sorgen bereitet: wenn ich buffer in einem synchronized() Block habe, und innerhalb dessen wait() aufrufe,
Du kannst/darfst wait() nur innerhalb einen synchronized-blocks aufrufen.

kann es dann überhaupt sein, dass ich an einer anderen Stelle die ebenfalls mit buffer synchronized() mit notify[All]() wecke?
Ja genau.

try catch muss tatsächlich so gesetzt werden. denn sobald eine InterruptedException aufgefangen wird, bedeutet das, dass dein Programm sich selbst beenden soll oder wird.
Dann solltest du das den try-block allerdings ausserhalb while(run) setzen resp in wirte die Exception `nach oben' weitergeben.

[Captain America]

Moment, moment, write(String) wird doch sicher von einem anderen Thread aus aufgerufen, oder nicht?

Du kannst/darfst wait() nur innerhalb einen synchronized-blocks aufrufen.

Ja genau.

Dann solltest du das den try-block allerdings ausserhalb while(run) setzen resp in wirte die Exception `nach oben' weitergeben.

ja mumpitz schrieb ich. es greifen natürlich mehrere threads darauf zu. und richtig ist auch, dass die while / trys vertauscht werden müssen, ich habe es schlampig hier rein kopiert.

ein testlauf und ein realer einsatz funktionieren einwandfrei. scheint wohl doch alles in butter zu sein, thx fürs lesen :uup: