ConcurrentModificationException nicht nachvollziehbar

[DrLoad]

Hi,

zu dieser Exception gibts ja eine Menge Lesestoff, ich habe jedoch einen Fall, wo sie meines Wissens nicht auftreten dürfte, es aber trotzdem tut. Der Code ist nichtmal besonders kompliziert:

	private Map<Integer,Scriptable> scripts = Collections.synchronizedMap(new HashMap<Integer, Scriptable>());

	@Override
	public void run() {
		while( true ) {
			synchronized(scripts){
				Vector<Scriptable> c = new Vector<Scriptable>();
				c.addAll(scripts.values());
				for(Scriptable script : c){
					if( script.isFinished()) scripts.remove(new Integer(script.getId()));
				}				
			}
			try {
				Thread.sleep( 500L );
			}
			catch( Exception x ) {
				x.printStackTrace(System.out);
			}
		}		
	}

	public void addScript( Scriptable script ){
		synchronized(scripts){
			scripts.put(new Integer(script.getId()), script);
		}
	}

Die ConcurrentModificationException tritt ja dann auf, wenn
- mehrere Threads auf eine Resource zugreifen oder
- ein einziger Thread eine Liste o. ä. verändert, während er über sie iteriert.

Beide Fälle meine ich im Code ausgeschlossen zu haben. Die kritischen Bereiche habe ich in einem synchronized - Block und Elemente werden aus der Original Map entfernt, während über eine vorher angelegte Kopie iteriert wird.

Habe ich einen Stolperstein übersehen?

Ich bin für jede Hilfe dankbar.

 

[SlaterB]

da du selber
synchronized(scripts){
benutzt, kannst du dir Collections.synchronizedMap() sparen,
die Methoden der Map werden nie für sich aufgerufen, sondern bei dir immer in einem sicheren Block

ich behaupte jetzt erstmal auch, dass das so sicher ist, treten bei dir Exceptions auf?
es fehlt ein vollständiges Programm, mit Thread-Klasse, main-Methode und Gesamtablauf

 

[DrLoad]

Danke für die Antwort. Ja, die Exception sieht bei mir folgendermaßen aus:

Exception in thread "Scripting-Thread" java.util.ConcurrentModificationException
        at java.util.HashMap$HashIterator.nextEntry(HashMap.java:793)
        at java.util.HashMap$ValueIterator.next(HashMap.java:821)
        at scripts.ScE.run(ScE.java:19)
        at java.lang.Thread.run(Thread.java:619)

Den Rest des Codes hielt ich für unwesentlich, hier aber nochmal die vollständige Klasse. Der Thread wird indirekt von der JVM gestartet und läuft vor sich hin, während andere Threads darauf zugreifen.

public class ScE implements Runnable {

	private Thread runner = null;

	private static ScE instance =  new ScE();

	private Map<Integer,Scriptable> scripts = Collections.synchronizedMap(new HashMap<Integer, Scriptable>());

	private ScE(){
		runner = new Thread(this, "Scripting-Thread");
        runner.start();  
	}

	@Override
	public void run() {
		while( true ) {
			synchronized(scripts){
				Vector<Scriptable> c = new Vector<Scriptable>();
				c.addAll(scripts.values());
				for(Scriptable script : c){
					if( script.isFinished()) scripts.remove(new Integer(script.getId()));
				}				
			}
			try {
				Thread.sleep( 500L );
			}
			catch( Exception x ) {
				x.printStackTrace(System.out);
			}
		}		
	}

	public void addScript( Scriptable script ){
		synchronized(scripts){
			scripts.put(new Integer(script.getId()), script);
		}
	}

	public static ScE getInstance(){
		return instance;
	}	
}

P.S. Laut Doku habe ich es so verstanden, dass man trotz "Collections.synchronizedMap" alle Zugriffe auf die gelieferte Map in synchronized Blöcken kapseln soll.

 

[KrokoDiehl]

Kann es sein, dass folgender Block
[JAVA=18]
Vector<Scriptable> c = new Vector<Scriptable>();
c.addAll(scripts.values());
for(Scriptable script : c){
if( script.isFinished()) scripts.remove(new Integer(script.getId()));
}
[/code]
dennoch deinen Vektor verändert? Denn

c.addAll()

fügt keine Kopien, sondern Referenzen deiner Scriptable-Objekte ein. Durch das

scripts.remove()

wird also am Ende auch dein Vektor

c

beeinflusst.
Naja, das ist meine Vermutung, wobei ich mir da eher unsicher bin.
Zum Testen kannst du aber mal folgende Variante probieren:

Vector<Integer> toDelete = new Vector<Integer>();
for (Scriptable script : scripts)
{
    if (script.isFinished())
        toDelete.add(script.getID());
}

// nun das Entfernen
for (Integer id : toDelete)
    scripts.remove(id);

 

[SlaterB]

@DrLoad
na die Klasse sagt für sich nunmal nichts über den Gesamtablauf aus,
hier mal ein Testprogramm:

public class Test
{

    public static void main(String[] args)
        throws Exception
    {
        ScE e = ScE.getInstance();
        while (true)
        {
            e.addScript(new Scriptable());
            System.out.println("added");
            Thread.sleep(100);
        }
    }
}


class ScE
    implements Runnable
{

    private Thread runner = null;

    private static ScE instance = new ScE();

    private Map<Integer, Scriptable> scripts = Collections.synchronizedMap(new HashMap<Integer, Scriptable>());

    private ScE()
    {
        runner = new Thread(this, "Scripting-Thread");
        runner.start();
    }

    public void run()
    {
        try
        {
            rrun();
        }
        catch (Exception x)
        {
            x.printStackTrace(System.out);
        }

    }

    private void rrun()
        throws Exception
    {
        long time = System.currentTimeMillis();
        while (true)
        {
            synchronized (scripts)
            {
                Vector<Scriptable> c = new Vector<Scriptable>();
                c.addAll(scripts.values());
                System.out.println((System.currentTimeMillis() - time) + " - start  run: " + scripts.size());
                for (Scriptable script : c)
                {
                    if (script.isFinished()) scripts.remove(new Integer(script.getId()));
                    Thread.sleep(50L);
                }
                System.out.println((System.currentTimeMillis() - time) + " - finish run: " + scripts.size());
            }
            Thread.sleep(500L);
        }
    }

    public void addScript(Scriptable script)
    {
        synchronized (scripts)
        {
            scripts.put(new Integer(script.getId()), script);
        }
    }

    public static ScE getInstance()
    {
        return instance;
    }
}


class Scriptable
{
    static int count = 0;

    private int id = count++;

    public boolean isFinished()
    {
        return Math.random() < 0.2;
    }

    public int getId()
    {
        return id;
    }
}

16 - start  run: 0
16 - finish run: 0
added
added
added
added
added
516 - start  run: 5
766 - finish run: 2
added
added
added
added
added
1266 - start  run: 7
1625 - finish run: 5
added
added
added
added
added
2125 - start  run: 10
2625 - finish run: 7
added
added
added
added
added
3125 - start  run: 12
3735 - finish run: 10
added
added
added
added
added
4235 - start  run: 15
5000 - finish run: 12
added
added
added
added
added
5500 - start  run: 17
6360 - finish run: 16
added
added
added
added
added
6860 - start  run: 21
7922 - finish run: 15
added
added
added
added
added
8422 - start  run: 20
9438 - finish run: 17
added
added
added
added
added
9938 - start  run: 22
11063 - finish run: 17
added
added
added
added
added
11563 - start  run: 22
12672 - finish run: 13
added
added
added
added
added
13172 - start  run: 18
14094 - finish run: 16
added
added
added
added
added
14594 - start  run: 21
...

keine Exceptions, und ich persönlich habe leider weiterhin auch keine Fantasie, mir einen möglichen Grund für eine Exception auszudenken,
mir scheint das narrensicher

ich könnte gar behaupten, dass die Exception, so wie da steht (15:19), gar nicht auftreten kann,
den StackTrace bekomme ich bei

for (Scriptable script : scripts.values())

wenn alles synchronisierende wegfällt, denn bei derartigen Code landet man von der Runnable-Methode aus direkt im Iterator,

wenn aber addAll() benutzt wird, dann müsste das im StackTrace als eine Zeile auftauchen

-----

> P.S. Laut Doku habe ich es so verstanden
glaube ich gerne, aber ist ja kein entscheidendes Kriterium


> Durch das scripts.remove() wird also am Ende auch dein Vektor c beeinflusst.

das glaube ich nicht

 

[DrLoad]

Ich bedanke mich herzlich für die Antworten und besonders bei dir SlaterB für die Mühe und den Test des Programms.
Tatsächlich hatte ich zwischenzeitlich eine ähnliche Lösung, wie sie KrokoDiehl vorgeschlagen hat in Verwendung (also mit Delete Liste), was aber nichts half, daher passt die Exception nicht so ganz zum Code .

Leider kann ich den Fehler nicht gezielt reproduzieren, aber ich bin schon mal froh, dass eurer Einschätzung nach das Ding soweit sicher sein sollte.

P.S. Was habe ich denn falsch verstanden?

 Map m = Collections.synchronizedMap(new HashMap());
      ...
  Set s = m.keySet();  // Needn't be in synchronized block
      ...
  synchronized(m) {  // Synchronizing on m, not s!
      Iterator i = s.iterator(); // Must be in synchronized block
      while (i.hasNext())
          foo(i.next());
  }

Muss m nicht auf jeden Fall im sync sein, wenn man iterieren will?

 

[SlaterB]

so wie es da steht ist beides richtig,
synchronized{ } für den Iterator,
keySet() sicher, da Map synchronisiert ist

wenn du aber keySet() sowie alle anderen Map-Methoden eh nur in synchronized{ }-Blocks aufrufst, wie in ScE der Fall,
dann ist es unnötig, extra eine Collections.synchronizedMap() zu verwenden, dann klappt es mit einer normalen Map genauso

 

[DrLoad]

Danke, wieder was gelernt!

Wird denn, wenn ich sowas habe:

for (Scriptable script : scripts.values())

intern ein Iterator erzeugt, d.h müßte das auch in einen synchronized Block?

Woanders habe ich gelesen, dass man mit "ConcurrentHashMap" (für den Preis von Effizienz) so eine Art Rundumsorglos Paket erhält was Threadsicherheit angeht. Würdet ihr das auch so sehen?

 

[SlaterB]

> müßte das auch in einen synchronized Block?

das müsste, und wenn nicht gibts wie gesagt genau deine Exception, abgesehen von rrun() bei mir:

public class Test
{
    public static void main(String[] args)
        throws Exception
    {
        ScE e = ScE.getInstance();
        while (true)
        {
            e.addScript(new Scriptable());
            System.out.println("added");
            Thread.sleep(100);
        }
    }
}

class ScE
    implements Runnable
{
    private Thread runner = null;
    private static ScE instance = new ScE();
    private Map<Integer, Scriptable> scripts = new HashMap<Integer, Scriptable>();

    private ScE()
    {
        runner = new Thread(this, "Scripting-Thread");
        runner.start();
    }

    public void run()
    {
        try
        {
            rrun();
        }
        catch (Exception x)
        {
            x.printStackTrace(System.out);
            System.exit(0);
        }
    }

    private void rrun()
        throws Exception
    {
        long time = System.currentTimeMillis();
        while (true)
        {
            Vector<Scriptable> c = new Vector<Scriptable>();
            c.addAll(scripts.values());
            System.out.println((System.currentTimeMillis() - time) + " - start  run: " + scripts.size());
            for (Scriptable script : scripts.values())
            {
                if (script.isFinished()) scripts.remove(new Integer(script.getId()));
                Thread.sleep(50L);
            }
            System.out.println((System.currentTimeMillis() - time) + " - finish run: " + scripts.size());

            Thread.sleep(500L);
        }
    }

    public void addScript(Scriptable script)
    {
        scripts.put(new Integer(script.getId()), script);
    }

    public static ScE getInstance()
    {
        return instance;
    }
}


class Scriptable
{
    static int count = 0;

    private int id = count++;

    public boolean isFinished()
    {
        return Math.random() < 0.2;
    }

    public int getId()
    {
        return id;
    }
}

added
0 - start  run: 1
47 - finish run: 1
added
added
added
added
added
547 - start  run: 6
added
java.util.ConcurrentModificationException
	at java.util.HashMap$HashIterator.nextEntry(HashMap.java:793)
	at java.util.HashMap$ValueIterator.next(HashMap.java:822)
	at test.ScE.rrun(Test.java:65)
	at test.ScE.run(Test.java:46)
	at java.lang.Thread.run(Thread.java:619)

----

Iterator natürlich, so ist das bei for-each-Schleifen

-----

ich persönlich halte von Collections.synchronizedMap() nichts, allein schon weil ich nicht weiß, ob auch das Durchlaufen solcher Iteratoren sicher ist,
nach deinem Posting von 16.18 (// Must be in synchronized block) siehts da ja schlecht aus,

meine Philiosophie ist bisher, dass das Synchronisieren derart selten und so wichtig ist,
dass man da ruhig selber synchronized{} schreiben kann, gar immer eine eigene Klasse wie deine Klasse ScE,
auf die Map sollte niemand direkt zugreifen dürfen, sondern nur vorgegebene Methoden wie ScE.add(),
und dann ist es ja leicht da selber synchronized{}-Blöcke hinzuschreiben,

Collections.synchronizedMap() kann ich persönlich nichts abgewinnen,

falls ConcurrentHashMap was anderes ist, kann ich dazu nichts sagen, außer das gleiche, falls es ähnlich ist