# RMI Produzent-Verbraucher-Problem - Code review



## cspecial (9. Jul 2019)

Hallo. Ich möchte verteilte Anwendungen reservieren, wobei die Clienten als Konsumenten, die auf einen serverseitigen Puffer zugreifen, auftreten. Realisiert wird die Kommunikation über die RMI-Schnittstelle.
Der Server besitzt zudem einen Produzent, der den Puffer füllt. Die maximale Puffergröße ist 20.

Die Aufgabe habe ich mir selbst zusammengestellt und sollte nur dazu dienen, den Wissensstand zu überprüfen. Bitte die Ausführungen der Ausnahmebehandlungen, wie sie im Code auftreten, unberücksichtigt lassen und voraussetzen, dass die Objekte serverseitig registriert wurden.

Vielen Dank im Voraus!


```
public class Buffer

{

   private Vector<Integer> buffer;

   private Semaphor mutex;

   private boolean flag1;

   private boolean flag2;   


   public Buffer()

   {

      buffer = new Vector<Integer> buffer;

      mutex = new Semaphor(1);

      flag1 = false;

      flag2 = false;

   }


   public synchronized Integer get()

   {

      if (buffer.size() == 0)

      {

         flag1 = true;

 

         if(flag2)

         {

            notify();

         }


         try

         {

            wait();

         } catch(Exception e)

         {}

       }


       mutex.p();

       Integer temp = buffer.lastElement();

       buffer.remove(temp);

       mutex.v();

 

       return temp;

    }


    public synchronized void put(Integer a)

   {

      if (buffer.size() == 20)

      {

         flag2 = true;

 

         if(flag1)

         {

            notify();

         }


         try

         {

            wait();

         } catch(Exception e)

         {}

       }


       mutex.p();

       buffer.add(a);

       mutex.v();

    }

}

          



public interface Consumer extends Remote

{

   public Integer get() throws RemoteException;

}




public class ConsumerImpl extends UnicastRemoteObject implements Consumer

{

   private Buffer buffer;

  

   public ConsumerImpl(Buffer buff) throws RemoteException

   {

      buffer = buff;

   }


   public Integer get() throws RemoteException

   {

      return buffer.get();

   }

}




public class Producer extends Thread

{

   private Buffer buffer;


   public Producer(Buffer buff)

   {

      buffer = buff;

      this.start();

   }


   public void run()

   {

      Integer a = 0;

 

      while (true)

      {

         buffer.put(a);

 

         try

         {

            sleep(1000);

         }

         catch (Exception e) {}

      }

   }

}




public class Server

{

   public static void main(String[] args)

   {

      try

      {

         Buffer buffer = new Buffer();

         ConsumerImpl c1 = new ConsumerImpl(buffer);

         Producer prod = new Producer(buffer);

         Naming.rebind("Server1", c1);

       }

       catch(Exception e)

       {

          return;

       }

   }

}




public class Client_J

{

   public static void main(String[] args)

   {

      try

      {

         Consumer c1 = (Consumer) Naming.lookup("rmi://" + args[0] +   "/Server1");


         while(true)

         {

            System.out.println(c1.get());

         }

       }

       catch(Exception e)

       {

           return;

       }

   }

}
```


----------



## mihe7 (9. Jul 2019)

cspecial hat gesagt.:


> Vielen Dank im Voraus!


Wofür? Ah, im Titel...


----------



## mrBrown (9. Jul 2019)

Als erste Anmerkung: Die Formatierung ist für ein Forum einer der ungeeignetsten


----------



## cspecial (9. Jul 2019)

mihe7 hat gesagt.:


> Wofür? Ah, im Titel...


Ist der Code korrekt?


----------



## mihe7 (9. Jul 2019)

Habe mir jetzt nur die Buffer-Klasse überflogen: schlecht wäre auch nicht, wenn flag1 und flag2 vernünftige Namen hätten und man wissen würde, welche Klassen Du verwendest (wo kommt denn Semaphor her? Ist Vector java.util.Vector?)


----------



## cspecial (9. Jul 2019)

mihe7 hat gesagt.:


> Habe mir jetzt nur die Buffer-Klasse überflogen: schlecht wäre auch nicht, wenn flag1 und flag2 vernünftige Namen hätten und man wissen würde, welche Klassen Du verwendest (wo kommt denn Semaphor her? Ist Vector java.util.Vector?)


Genau, java.util.Vector.

Sempahor:

```
public class Semaphor
{
    private int value;

    public Semaphor(int val)
    {
        value = val;
    }

    public synchronized void p()
    {
        while (value < 0)
        {
            try
            {
                wait();
            }
            catch (Exception e) {}
        }

        value--;
    }
 
 
    public synchronized void v()
    {
        if (value < 0)
        {
            notifyAll(); //bzw. notify()
        }
     
        value++;
    }
}
```

Edit: flag1 wird gesetzt, sobald mindestens ein Konsument im Blockiert-Zustand ist (Puffer leer). Das flag2 wird gesetzt, sobald der Produzent blockiert wird (Puffer voll).

Edit2: Habe vergessen, die Flags zu entfernen (Anpassung der beiden Methoden in der Buffer-Klasse)

```
public synchronized Integer get()
   {
      while (buffer.size() == 0)
      {
         flag1 = true;

         if(flag2)
         {
            notify();
            flag2 = false;
         }

         try
         {
            wait();
         } catch(Exception e) {}

       }

       mutex.p();
       Integer temp = buffer.lastElement();
       buffer.remove(temp);
       mutex.v();

       return temp;
    }


    public synchronized void put(Integer a)
   {

      if (buffer.size() == 20)
      {
         flag2 = true;
        
         if(flag1)
         {
            notifyAll();
            flag1 = false;
         }

         try
         {
            wait();
         } catch(Exception e)         {}
       }

       mutex.p();
       buffer.add(a);
       mutex.v();
    }
```


----------



## mrBrown (9. Jul 2019)

Noch/Nur zu Buffer:

- flag1 und flag2 bleiben immer true (oder ich überseh das setzen auf false).
- die beiden sollten bessere Namen bekommen...
- `buffer = new Vector<Integer> buffer;` sollte vermutlich eher `buffer = new Vector<>(20);` sein?
- p() und v() wäre mit try-finally vermutlich besser (oder auch mit try-with-resources)
- warum Vector und nicht irgendeine Queue? Die Threadsicherheit von Vector brauchst du nicht, da kümmerst du dich ja selbst drum
- Keine Ahnung ob ich das hier grad richtig lese/verstehe, aber wenn der Buffer einmal voll war, wird er erst wieder weiter aufgefüllt, wenn er danach wieder leer war?

_- warum eine (selbstgeschriebene) Semaphore und nicht einfach irgendeinen simplen Lock aus der Concurrent-API?
- warum nicht statt den händischen get/put eine passende Queue (zB BlockingQueue dürfte ein passender Ersatz sein, wenn mich nichts täuscht)_


----------



## mihe7 (9. Jul 2019)

Dazu: mit Semaphore(1) kann p() zweimal aufgerufen werden, bevor blockiert wird.


----------



## Xyz1 (9. Jul 2019)

Kann so etwas nicht vernünftig formatiert geposted werden? Dann hätte man vielleicht auch Lust, ein kostenloses Review zu machen.


----------



## cspecial (10. Jul 2019)

mrBrown hat gesagt.:


> Noch/Nur zu Buffer:
> 
> - flag1 und flag2 bleiben immer true (oder ich überseh das setzen auf false).
> - die beiden sollten bessere Namen bekommen...
> ...



Zu 1): Nein, du hattest es richtig erkannt. Ich hatte es aber in meinem letzten Post noch einmal ausgebessert.
Zu 2): Da stimme ich zu, ich hätte die Funktion deutlicher kennzeichnen sollen.
Zu 3): Ja, das wäre die bessere Wahl, hatte es dann aber allgemein gelassen.
Zu 4): Die Datenstruktur war vorgesehen.
Zu 5): Der Puffer kann jederzeit, sobald er nicht voll ist, aufgefüllt werden.

Ich hätte noch dazu schreiben sollen, dass es mir bei der Aufgabe nur darum ging, das Produzent-Verbraucher Problem im RMI-Zusammenhang richtig gelöst zu haben.
Sollte das Programm ansonsten ordnungsgemäß laufen?



mihe7 hat gesagt.:


> Dazu: mit Semaphore(1) kann p() zweimal aufgerufen werden, bevor blockiert wird.


Ja, es müsste dann stattdessen Semaphor(0) sein.

Vielen Dank für eure Hilfe! Ich wollte den Code noch einmal formatieren, hatte aber keine Möglichkeit mehr dazu gehabt, nachdem die ersten Antworten veröffentlicht wurden.


----------



## mrBrown (10. Jul 2019)

cspecial hat gesagt.:


> Zu 1): Nein, du hattest es richtig erkannt. Ich hatte es aber in meinem letzten Post noch einmal ausgebessert.
> Zu 2): Da stimme ich zu, ich hätte die Funktion deutlicher kennzeichnen sollen.
> Zu 3): Ja, das wäre die bessere Wahl, hatte es dann aber allgemein gelassen.



Irgendeinen Punkt hast du übersprungen, entweder 3 oder 4...



cspecial hat gesagt.:


> Zu 4): Die Datenstruktur war vorgesehen.



Das bezieht sich vermutlich auf den Vector? Warum siehst du denn bei selbst gestellten Aufgaben unsinnige Datenstrukturen vor?



cspecial hat gesagt.:


> Zu 5): Der Puffer kann jederzeit, sobald er nicht voll ist, aufgefüllt werden.


Vielleicht missversteh ich grad was, aber:

Angenommen, es sind grad 20 Einträge drin, und es wird erneut put aufgerufen.
Das if trifft zu, flag1 ist false, er landet also einfach beim wait().

Aus dem wait wacht der erst beim notify auf, und das passiert nur, wenn vorher in get die Liste einmal leer war.



Ich glaube auch, dass die flags falsch sind.
flag1 bedeutet laut dir, dass "mindestens ein Konsument im Blockiert-Zustand ist (Puffer leer)", aktuell ist der aber wahr, wenn irgendwann mal der Puffer leer war, und dann solange, wie der Puffer nie ganz voll war.
Für flag2 gilt umgekehrt das gleiche.



cspecial hat gesagt.:


> Sollte das Programm ansonsten ordnungsgemäß laufen?


Ich würde nicht drauf vertrauen...
Schreib dir einfach mal 'nen Test für den Buffer, um sicher zu gehen, dass der ohne RMI der läuft


----------



## cspecial (10. Jul 2019)

mrBrown hat gesagt.:


> Irgendeinen Punkt hast du übersprungen, entweder 3 oder 4...
> 
> 
> 
> ...



Warum sollte die Datenstruktur unsinnig sein? Sie arbeitet wie ein Stack und erfüllt ihren Zweck. Wie gesagt, mir geht es eher um den RMI-Aspekt.

Ansonsten stimmen die Aussagen zu den Flags, hier mal eine kleine Verbesserung:

```
public synchronized Integer get()
   {
      while (buffer.size() == 0)
      {
         flag1 = true;

         if(flag2)
         {
            notifyAll();
            flag2 = false;
         }

         try
         {
            wait();
         } catch(Exception e) {}

       }

       mutex.p();
       Integer temp = buffer.lastElement();
       buffer.remove(temp);
       mutex.v();
        
       if (flag2)
       {
          notifyAll();
          flag2 = false;
       }
      
       return temp;
    }


    public synchronized void put(Integer a)
   {
       mutex.p();
       buffer.add(a);
       mutex.v();
      
       if (flag1)
       {
          notifyAll();
          flag1 = false;
       }
      
       if (buffer.size() == 20)
       {
         flag2 = true;
        
         try
         {
            wait();
         } catch(Exception e)         {}
       }
    }
```

Es wäre auch besser, für den Produzenten und die Konsumenten jeweils eine Lock-Variable zu definieren, dann müssten nicht nach Erkennen eines gesetzten Flags alle aus dem blockiert-Zustand in rechenbereit überführt werden, sondern nur die, für die es gedacht ist.

Gruß
cspecial


----------



## mrBrown (10. Jul 2019)

cspecial hat gesagt.:


> Warum sollte die Datenstruktur unsinnig sein? Sie arbeitet wie ein Stack und erfüllt ihren Zweck.


Aus dem Javadoc zu Vector: 


			
				https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Vector.html hat gesagt.:
			
		

> If a thread-safe implementation is not needed, it is recommended to use ArrayList in place of Vector.



Wenn du Stack-Verhalten willst, würde ich statt Vector lieber Stack nutzen - da drückt der Typ schon aus, als was es gedacht ist (und `temp` ist dann überflüssig).
Wobei dann da wiederum auch gilt:


			
				https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Stack.html hat gesagt.:
			
		

> A more complete and consistent set of LIFO stack operations is provided by the Deque interface and its implementations, which should be used in preference to this class.






cspecial hat gesagt.:


> Wie gesagt, mir geht es eher um den RMI-Aspekt.


Der ist vermutlich passend, aber das sind ja kaum drei Zeilen...



cspecial hat gesagt.:


> Ansonsten stimmen die Aussagen zu den Flags, hier mal eine kleine Verbesserung:


Die Begrenzung auf 20 gilt aber jetzt nur so lange, wie genau ein Thread als Producer fungiert. Bei 1+N Producern liegt die Obergrenze bei 20+N. Verbesserung würd ich das also nicht nennen 



cspecial hat gesagt.:


> Es wäre auch besser, für den Produzenten und die Konsumenten jeweils eine Lock-Variable zu definieren, dann müssten nicht nach Erkennen eines gesetzten Flags alle aus dem blockiert-Zustand in rechenbereit überführt werden, sondern nur die, für die es gedacht ist.


Sind die Flags überhaupt nötig? Theoretisch könnte man die weglassen, als "Flags" reichen die Tests auf min- und maximale Größe. Nach jedem rausnehmen und reinlegen weckt man einfach alle auf, die prüfen jeweils ob sie was machen können. Bin grad nur unterwegs, vielleicht hab ich da auch grad einen Denkfehler drin...die Flags dürften dabei uU nur einen kleinen Performance-Vorteil bringen, aber ob der die Komplexität wert ist?


Wofür ist eigentlich der extra Lock um put und lastElement da? Die Methode selbst ist doch schon synchronized.


----------

