Threads ArrayList in Thread nicht modifizierbar?

[renwal]

Hallo!

Ja, ich schon wieder... Ich hoffe, ich nerv' euch nicht langsam, aber ich hänge schon wieder fest...

Ich habe eine Software programmiert bzw. bin dabei, die einen LEGO Mindstorms NXT-Roboter steuern kann. Hintergrund: Ich hab ein Boot gebaut, das über Bluetooth gesteuert wird und über das man via Unterwasserkamera Bilder in einem beliebigen Gewässer machen kann.
Jetzt erzeugen verschiedene Steuerungs-Schaltflächen und -Regler sowie mehrere laufende Aktualisierungen (Signalstärke, Akkustatus, Sensordaten, etc.) Steuerbefehle, die, um nicht bei der Übertragung zu kollidieren, an einen Thread übergeben werden, der die Befehle dann nacheinander sendet (und sich danach auch um die Umsetzung im UI kümmert).
Kommt jetzt ein neuer Eintrag in die Liste zur Abarbeitung, dann quittiert mir der Debugger das auch, und zwar so lange, wie die Methode zum Hinzufügen läuft. Beginnt der Thread dann mit der Abarbeitung (Prüfung auf size!=0 jede 1/2 Sekunde, nicht optimal, ich weiß), dann zeigt der Debugger an, die ArrayList sei leer. Was mache ich falsch?

Hier ist mein Code:

package communication;

import icommand.nxt.Sound;

import java.util.ArrayList;

import javax.swing.JOptionPane;

public class CommandDispatcher_v2 extends Thread {

    private ArrayList<Command> commands = new ArrayList<Command>();
    private boolean ignoreNew = false;
    private boolean breakRequested = false;
    private int commandCount = 0;

    @Override
    public void run()
    {
	try
	{
	    while (!isInterrupted())
	    {
		System.out.println("check commands list");
		// Prüfen, ob es "etwas zu tun" gibt
		if (commands.size() != 0)
		{
		    // Alles abarbeiten, bis es nichts mehr zu tun gibt
		    while (commands.size() != 0)
		    {
			// Abbrechen, wenn das beantragt wurde
			if (isInterrupted())
			{
			    // Alle ausstehenden Befehle löschen, damit isIdle()
			    // true zurückgibt
			    commands.clear();
			    breakRequested = true;
			    break;
			}
			// Den ersten, sprich ältesten, Befehl ausführen
			exec(commands.get(0));
			// Den ausgeführten Befehl löschen
			commands.remove(0);
		    }
		    if (breakRequested)
			break;
		}
		/*
		 * Das try/catch im try/catch ist deshalb da, damit ein
		 * Interrupt nicht gleich den Thread "gewaltsam" abbricht.
		 */
		try
		{
		    // Eine halbe Sekunde lang warten...
		    Thread.sleep(500);
		}
		catch (final InterruptedException interrupt)
		{
		    // Interrupt-Flag muss nach der Exception neu gesetzt werden
		    interrupt();
		}
		// ... und wieder prüfen
	    }
	    System.out.println("CommandDispatcher interrupted");
	}
	catch (final Exception e)
	{
	    JOptionPane
		    .showMessageDialog(
			    null,
			    "Das Befehls-Abarbeitungssystem hat einen Fehler festgestellt\r\n"
				    + "und musste beendet werden. Die Verbindung zum NXT wird jetzt getrennt.",
			    "Kritischer Absturz", JOptionPane.ERROR_MESSAGE);
	}
    }

    /**
     * Führt den übergebenen Befehl aus.
     * 
     * @param command
     *            Ein Kommando-Objekt, das die Steuerdaten enthält
     */
    private void exec(Command command)
    {
	System.out.println("exec command");
	// TODO Hier müssen die Befehle rein
	if (command.getCommand().equals("playSound"))
	    Sound.playTone(command.getParam(0), command.getParam(1));
	// Zähler für ausgeführte Kommandos hochzählen
	commandCount++;
    }

    /**
     * Fügt Kommandos in die Warteliste ein.
     * 
     * @param command
     *            Ein Kommando-Objekt, das in die Warteliste eingefügt werden
     *            soll
     * @return true, wenn das Hinzufügen erfolgreich war, sonst false
     */
    public boolean addCommand(Command command)
    {
	// Kommando an die Warteschlange anfügen
	if (ignoreNew)
	    return false;
	commands.add(command);
	System.out.println("command added.");
	return true;
    }

    /**
     * Wird beim Beenden des Threads aufgerufen, damit noch alle Kommandos
     * abgearbeitet werden können, bevor der Thread mit interrupt() beendet
     * wird.
     * 
     * @param option
     *            Ob neue Kommandos ignoriert werden sollen
     */
    public void setIgnoreNew(boolean option)
    {
	ignoreNew = option;
    }

    /**
     * Prüft, ob der Thread gerade beschäftigt ist oder nicht.
     * 
     * @return true, wenn die Warteliste leer ist, d.h. der Thread nichts
     *         "zu tun" hat
     */
    public boolean isIdle()
    {
	// Prüfen, ob die Kommandoliste leer ist
	if (commands.size() == 0)
	    return true;
	return false;
    }

    /**
     * Gibt die Anzahl an Kommandos in der Warteliste zurück
     * 
     * @return Die Anzahl an Kommandos in der Warteliste
     */
    public int getCommandCount()
    {
	System.out.println("getting command count (" + commandCount + ")");
	return commandCount;
    }

}

FindBugs habe ich übrigens schon drüber laufen lassen, der findet zwar was, aber nichts zu diesem Problem, außer "Write to static field ui.Remote.commandDispatcherv2Class from instance method new ui.Remote()". Aber das hat doch eigentlich nur mit der Initialisierung zu tun, oder?

 

[U2nt]

Ich würde mal sagen, dass du den Command an dem Index 0 löschst und dann nicht nach unten rutschen lässt. Dafür bietet sich übrigens eine

LinkedList

an. Bei der rutschen die Elemente, wenn du etwas removest, automatisch nach.

Außerdem würde ich die doppelte Bedingung von

commands.size() != 0

nur einmal schreiben, das if außenrum ist nicht notwendig.

 

[Spacerat]

ArrayLists sind im Gegensatz zu ihrem "Uropa" dem Vector nicht synchronisiert. Das bedeutet, man muss sie extern synchronisieren, wenn man sie vernünftig in Threads nutzen will. Dabei können aber recht schnell Deadlocks entstehen. Deswegen sollte man innerhalb eines Threads immer mit der Kopie einer ArrayList bzw. einer unsynchronisierten Collection (mit anderen Worten: eigentlich immer ) arbeiten.

public class CommandDispatcher_V3
{
  private final List<Command> commands = new ArrayList<Command>();

  private final Thread runner = new Thread("CommandDispatchThread")
  {
    @Override
    public void run()
    {
      List<Command> copy = new ArrayList<Command>();
      while(!isInterrupted()) {
        synchronized(commands) {
          copy.addAll(commands);
          commands.clear();
        }
        while(!copy.isEmpty()) {
          exec(copy.remove(0));
        }
        synchronized(this) {
          try {
            wait(500);
          } catch(InterruptedException e) {
            // ignore
          }
        }
      }
    }
  };

  {
    runner.start();
  }

  private exec(Command c)
  {
    // your code
  }

  public boolean addCommand(Command c)
  {
    synchronized(commands) {
      return commands.add(c);
    }
  }
}

Das Ganze ist zu diesem Zeitpunkt zwar ungetestet, sollte aber wie folgt funktionieren:
Erstens soll es egal sein, wann welcher Thread einen Command ausführen will, wenn überhaupt, es geht nur nacheinander. Nicht zuletzt deswegen wird bei "addCommand" schonmal die Liste synchronisiert. Der Zweite Grund ist der "runner"-Thread. Es dürfen keine Commands hinzugefügt werden, solange dieser die Liste zwecks eigener Abarbeitung kopiert. Deswegen synchronisiert "runner" die Liste seinerseits und arbeitet anschliessend mit der erstellten Kopie. Ist diese leer, wartet er 500ms auf neue Commands. Busy-Waiting scheint hier wahrhaftig das einzige zu sein, was funktioniert. Bei einer "wait/notify"-Konstellation kann es passieren, dass der letzte Command hinzugefügt wird, während der "runner" gerade eine Liste abarbeitet und von einem "notify" deswegen nichts mitbekäme.
@U2nt: Bei der ArrayList rutschen beim Entfernen die Folgeelemente auch nach oben nach.

 

[renwal]

Danke schonmal für die Antworten! Ich werde den Code von Spacerat nachher mal ausprobieren.

 

[GUI-Programmer]

@Spacerat: Ich hatte bereits schon so einen ähnlichen Code, allerdings war dieser nicht, bzw. an den falschen Stellen synchronisiert und beinhaltete auch kein Kopieren der List. Danke.

[OT]Hier mein Code (Bitte nicht verwenden!):

package order;

import java.util.ArrayList;
import java.util.List;

public class OrderHandler {
	private List<Order> orderList;
	
	public OrderHandler() {
		orderList = new ArrayList<Order>();
		
		Thread thread = new Thread() {
			public void run() {
				while(true) {
					if(orderList.size() == 0) {
						try {
							Thread.sleep(10);
						} catch(InterruptedException ex) {
							System.err.println(ex.getMessage());
							interrupt();
						}
					}
					List<Order> oldOrders = new ArrayList<Order>();
					for(int i=0; i<orderList.size(); i++) {
						Order order = orderList.get(i);
						order.handleOrder();
						oldOrders.add(order);
					}
					orderList.removeAll(oldOrders);
				}
			}
		};
		thread.start();
	}
	
	public void addOrder(Order order) {
		orderList.add(order);
	}
}

[/OT]

 

[renwal]

Hm... Ich steig da nicht durch. Wie erzeuge ich denn jetzt eine neue Instanz dieser Klasse? Es gibt ja gar keinen Konstruktor, nur dieses komische

{
  runner.start();
}

. Wie muss ich das denn jetzt machen?

PS: Ich hab den Fehler an einer ganz anderen Stelle gefunden. Ich hab zwei Instanzen der Klasse gehabt, die eine war per run() gestartet, die andere nicht. Aber genau an diese nicht gestartete Klasse hab ich die ganzen Befehle gesendet. Ich hab das jetzt geändert, jetzt läuft es auch mit meinem im ersten Beitrag genannten Code, wenn es auch ständig abstürzt.

 

[tfa]

Bevor man mit synchronized, wait und notify rumfrickelt, sollte man sich lieber die neuen Concurrency-Klassen ansehen, z.B. CopyOnWriteArrayList, oder ReadWriteLocks. Das steigert die Wahrscheinlich, dass die Sache nicht in die Hose geht.

@renwal: Das {runner.start();} ist ein dynamischer Initialisierer, der aufgerufen wird, wenn ein Objekt der Klasse erzeugt wird (hier wollte wohl jemand cool programmieren). Den Default-Konstruktor gibt es natürlich schon.

 

[renwal]

So, initialisieren geht jetzt, aber ich kann jetzt die setDaemon()- und interrupt()-Methode nicht mehr aufrufen. Muss die dann auch direkt in die CommandDispatcher-Klasse in den Initialisierer?

 

[Spacerat]

Auch wenn's jetzt läuft...
Jede Klasse hat einen Konstruktor, im Zweifelsfalle einen parameterlosen Defaultkonstruktor, das ist jedem Java-Programmierer bekannt. Was allerdings nicht jedem bekannt ist, sind diese obendrein noch namenlosen Instanztinitialisierer, welche überall, in jeder Klasse noch zusätzlich zu jedem anderen Konstruktor aufgerufen werden - weis gerade nicht ob vor oder nach dem eigentlichen Konstruktoraufruf. Da man bei anonym erweiterten Klassen keinen Konstruktor überschreiben kann, kann man auf diese Art trotzdem einige Initialisierungen vornehmen. Komisch ist das nicht, sondern hilfreich.
[EDIT]

hier wollte wohl jemand cool programmieren.

Nee, mit Cool hat des nix zu tun, aber mit Faul. :lol: ...obwohl, was man nicht alles tut, um eine öffentliche Methode zu verstecken. [/EDIT]
[EDIT]

Muss die dann auch direkt in die CommandDispatcher-Klasse in den Initialisierer?

Die beiden Methoden kannst du natürlich mit in die CommandDispatcher-Klasse implementieren und von dort dann an runner delegieren. Cool wäre es, wenn du setDaemon(true); direkt unter die start()-Methode im Initializer packst.[/EDIT]
[EDIT]Hier mal das Ganze, wie ich es mir vorgestellt habe. Diesmal sogar getestet. Beachte, das nun auch dieser komische Initializer dort steht, wo er ursprünglich hin sollte. Aber gewöhn' dir's nicht an, das kann übel enden.

import java.util.ArrayList;
import java.util.List;

public class CommandDispatcher
{
	private final List<Command> commands = new ArrayList<Command>();
 
	private final Thread runner = new Thread("CommandDispatchThread")
	{
		{
			start();
			setDaemon(true);
		}

		@Override
		public void run()
		{
			List<Command> copy = new ArrayList<Command>();
			while(!isInterrupted()) {
				synchronized(commands) {
					copy.addAll(commands);
					commands.clear();
				}
				while(!copy.isEmpty()) {
					exec(copy.remove(0));
				}
				synchronized(this) {
					try {
						wait(500);
					} catch(InterruptedException e) {
						// ignore
					}
				}
			}
		}
	};

	/**
	 * @param c  
	 */
	private void exec(Command c)
	{
		//your code
	}
 
	public boolean addCommand(Command c)
	{
		synchronized(commands) {
			return commands.add(c);
		}
	}

	public void stop()
	{
		runner.interrupt();
		try {
			runner.join();
		} catch(InterruptedException e) {
			// ignore
		}
	}
}

abstract class Command
{
}

[/EDIT]

 

[GUI-Programmer]

Oder alternativ dieser Code, leicht modifiziert von Spacerat´s ersten Code:

CommandDispatcher_V3:

package order;

import java.util.ArrayList;
import java.util.List;

/**
 * Class CommandDispatcher_V3
 * @author Spacerat
 */
public class CommandDispatcher_V3
{
  private final List<Command> commands = new ArrayList<Command>();
 
  private final Thread runner = new Thread("CommandDispatchThread")
  {
    @Override
    public void run()
    {
      List<Command> copy = new ArrayList<Command>();
      while(!isInterrupted()) {
        synchronized(commands) {
          copy.addAll(commands);
          commands.clear();
        }
        while(!copy.isEmpty()) {
          copy.remove(0).exec();
        }
        synchronized(this) {
          try {
            wait(500);
          } catch(InterruptedException e) {
            // ignore
          }
        }
      }
    }
  };
 
  {
    runner.start();
  }
 
  public boolean addCommand(Command c)
  {
    synchronized(commands) {
      return commands.add(c);
    }
  }
}

Command:

package order;

public interface Command {
	public void exec();
}

Und ne Klasse zum Testen, CommandTester:

package order;

public class CommandTester {
	public static void main(String[] args) {
		Command command1 = new Command() {
			public void exec() {
				System.out.println("Command 1 started");
				try {
					Thread.sleep(3000);
				} catch(InterruptedException ex) {
					ex.printStackTrace();
				}
				System.out.println("Command 1 finished\n");
			}
		};
		Command command2 = new Command() {
			public void exec() {
				System.out.println("Command 2 started");
				try {
					Thread.sleep(3000);
				} catch(InterruptedException ex) {
					ex.printStackTrace();
				}
				System.out.println("Command 2 finished\n");
			}
		};
		Command command3 = new Command() {
			public void exec() {
				System.out.println("Command 3 started");
				try {
					Thread.sleep(3000);
				} catch(InterruptedException ex) {
					ex.printStackTrace();
				}
				System.out.println("Command 3 finished\n");
			}
		};
		
		CommandDispatcher_V3 commandDispatcher = new CommandDispatcher_V3();
		commandDispatcher.addCommand(command1);
		commandDispatcher.addCommand(command2);
		commandDispatcher.addCommand(command3);
	}
}

 

[Spacerat]

Äeh... wenn man Command schon als Interface mit exec-Methode baut, dann doch wenigstens richtig XD

public interface Command<T>
{
  T execute(Object ... parameter) throws Throwable;
}

Nun sind wir im innersten des Command-DesignPatterns angekommen.
Das wollte ich oben in meinem letzten Beispiel eigentlich auch schon so machen, aber das hätte eine komplette Neugestaltung erfordert. Der CommandDispatcher wäre dann im Prinzip zur ersten Stufe einer Script-Engine geworden.

 

[GUI-Programmer]

@Spacerat: OK, jetzt isses wohl richtig "pro - mäßig"! :applaus:

 

[renwal]

Habt ihr's dann? Wird das jetzt ein Programmier-Wettstreit, wer die komplizierteste Lösung findet??

Nein, Spaß beiseite:

Wenn ich jetzt das Programm beende oder die Verbindung zum NXT-Gerät trenne, möchte ich mehrere Möglichkeiten anbieten, was mit der Warteliste passieren soll: Entweder wird der Thread nach dem Abschluss der aktuellen Aktion (Übertragung, UI-Update) beendet, oder es soll noch gewartet werden, bis die Liste fertig abgearbeitet wurde.

Kann ich dann folgenden Code benutzen:

[...]
    private final List<Command> commands = new ArrayList<Command>();
    
    private boolean ignoreNew = false;
    private boolean finishBeforeQuit = false;

    private final Thread runner = new Thread("CommandDispatchThread") {
	@Override
	public void run()
	{
	    final List<Command> copy = new ArrayList<Command>();
	    while (!isInterrupted() || (!copy.isEmpty() && finishBeforeQuit))
	    {
		synchronized (commands)
		{
		    copy.addAll(commands);
		    commands.clear();
		}
		while (!copy.isEmpty() && (!isInterrupted() && !finishBeforeQuit))
		    exec(copy.remove(0));
		synchronized (this)
		{
		    try
		    {
			wait(500);
		    }
		    catch (final InterruptedException e)
		    {
			// ignore
		    }
		}
	    }
	}
    };

    public void stop(boolean finish)
    {
	finishBeforeQuit = finish;
	ignoreNew = true;
        runner.interrupt();
        try {
            runner.join();
        } catch(InterruptedException e) {
            // ignore
        }
    }
[...]

PS: Damit ihr nicht weiter mit der Command-Klasse rumrätselt, hier ist der Code:

package communication;

/**
 * Stellt einen Objekttyp bereit, der Befehle und Parameter enthält, die an den
 * NXT übertragen werden oder in der in der Anwendung integrierten
 * Steuerungseinheit umgesetzt werden.
 * 
 * @author Rene
 * 
 * @version 1.0.0
 * 
 */

public class Command extends Object {

    private String command;
    private int[] parameterInt;

    /**
     * Erzeugt ein neues Kommando-Objekt mit Parametern.
     * 
     * @param command
     *            Der Name des Befehls
     * @param parameterInt
     *            Ein Array mit Integer-Parametern
     * 
     */

    public Command(String command, int[] parameter)
    {
	this.command = command;
	parameterInt = parameter;
    }

    /**
     * Erzeugt ein neues, parameterloses Kommando-Objekt.
     * 
     * @param command
     *            Der Name des Befehls
     * 
     */

    public Command(String command)
    {
	this.command = command;
    }

    /**
     * Legt den Befehlsnamen fest.
     * 
     * @param command
     *            Der Name des Befehls
     */
    public void setCommand(String command)
    {
	this.command = command;
    }

    /**
     * Gibt den Befehlsnamen zurück.
     * 
     * @return Der Name des Befehls
     */
    public String getCommand()
    {
	return command;
    }

    /**
     * Gibt das Parameter-Array zurück.
     * 
     * @return Das gesamte Parameter-Array
     */
    public int[] getParams()
    {
	return parameterInt;
    }

    /**
     * Gibt den Parameter mit dem Index i zurück
     * 
     * @param i
     *            Der Index i
     * @return Der Parameter mit dem Index i
     * @throws ArrayIndexOutOfBoundsException
     *             Falls kein Eintrag mit dem Index i existiert
     */
    public int getParam(int i) throws ArrayIndexOutOfBoundsException
    {
	return parameterInt[i];
    }

    /**
     * Legt die Parameterliste fest.
     * 
     * @param params
     *            Ein Array mit Integer-Parametern
     */
    public void setParams(int[] params)
    {
	parameterInt = params;
    }

}

 

[Spacerat]

Dann wäre es doch am einfachsten, wenn man in der stop-Methode zunächst ignoreNew auf true setzt, einen SpzialCommand hinzufügt, aus welchem man ein definiertes Ergebnis auslesen kann, welches in der exec-Methode gesetzt werden muss und erst wenn man dieses Ergebnis hat den Thread beendet.

 

[renwal]

... und jetzt nochmal so, dass ein Java-Anfänger das versteht, bitte. :bahnhof:

 

[Spacerat]

Mal schauen, ob ich's aus dem Stehgreif hinbekomme.

public void stop(boolean finish) {
  Commands end = null;
  synchronized(commands) {
    ignoreNew = true;
    if(finish) {
      end = new Command("interrupt", new int[] {0});
      commands.add(end);
    }
  }
  if(end != null) {
    while(end.getParam(0) == 0) {
      try {
        Thread.sleep(500);
      } catch(InterruptedException e) {
        // ignore
      }
    }
  }
  runner.interrupt();
  try {
    runner.join();
  } catch(InterruptedException e) {
    // ignore
  }
}

private void exec(Command c) {
  if(c.getCommand().equals("interrupt")) {
    c.setParam(0, -1); // müsste noch implementiert werden
  }
}

 

[renwal]

Klingt logisch,

Commands

müsste aber wohl eher

Command

heißen. Aber warum erst

interrupt()

aufrufen, wenn der Thread fertig ist? Gibt es Probleme, wenn der Abbruch nach dem interrupt-Befehl erst mit einer großen Verzögerung folgt?

 

[Spacerat]

Oh ja, die gibt es... dein "interrupt"-Commando wird dann möglicherweise nicht mehr ausgeführt. Der Thread ist ja noch nicht fertig, er wartet immer noch im halben-Sekunden-Zyklus auf neue Commands. Diese Schleife sollte man erst unterbrechen, wenn man eine Reaktion vom definitiv letzten Command bekommen hat.

 

[renwal]

Gut, ich werde mal versuchen, das anhand deines Code-Beispiels umzusetzen. Wann ich die Woche über dazu Zeit finde, kann ich allerdings noch nicht sagen.

 

[renwal]

So, eingebaut habe ich es, ausprobieren werde ich es wahrscheinlich morgen oder am Wochenende.

 

[renwal]

Ich habe den neuen Code gerade ausprobiert und festgestellt, dass das mit dem beenden-Kommando einwandfrei funktioniert, aber der Aufruf von

interrupt()

danach rein gar nichts tut. Das Programm bleibt in

runner.join()

hängen und wartet unendlich lange, bis man es von Hand abbricht.

Woran das liegt:

[...]
synchronized (this)
{
    try
    {
        wait(500);
    }
    catch (final InterruptedException e)
    {
        // ignore
    }
}
[...]

Wird jetzt

interrupt()

aufgerufen, dann erwischt es den Thread höchstwahrscheinlich in diesem Code-Teil. Es wird eine InterruptedException ausgelöst, der interrupt-Status wird zurückgesetzt und der Thread läuft weiter, als wäre nichts gewesen.
Richtig müsste es sein:

[...]
synchronized (this)
{
    try
    {
        wait(500);
    }
    catch (final InterruptedException e)
    {
        interrupt();
    }
}
[...]

Eingefügt, ausprobiert, geht!

Danke für die Hilfe!!

 

[El_Rabbit]

Geiles Projekt:toll::applaus:

 

[Spacerat]

Na was ist das denn? Dann konnte ich ja auch noch was lernen. Danke