Getting Concurrent Modifiraction Exception. Where is the prob.lem? (code)

Question

Im writing a simple game in Java. Here is main code:

public class MainPanel extends JPanel {
    private Player player = new Player(100, 100, 3, 3);
    private Point2D targetPoint = new Point2D.Float(130, 350); //Pos on begin
    private ArrayList<Beam> beams = new ArrayList<Beam>();

    public MainPanel() {
        setPreferredSize(new Dimension(300, 400));

        addMouseMotionListener(new MouseMotionHandler());
        //Add shortcuts
        makeShortcut("player.BM1", "F1", new SetBeamModeAction(1));
        makeShortcut("player.BM2", "F2", new SetBeamModeAction(2));

        //Start threads
        Thread t = new Thread(new PlayerMoveRunnable());
        t.start();
        Thread t2 = new Thread(new PlayerShootRunnable());
        t2.start();
    }

    public void paintComponent(Graphics g) {
        Graphics2D g2 = (Graphics2D)g;
        g2.setColor(Color.BLACK);
        g2.fillRect(0, 0, 300, 400);
        //Draw player
        g2.drawImage(player.getImage(), (int)player.getX(), (int)player.getY(), null);
        //Draw beams
        for (Beam beam : beams) {
            g2.drawImage(beam.getImage(), (int)beam.getX(), (int)beam.getY(), null);
        }
    }

    //Thread running all the time
    private class PlayerMoveRunnable implements Runnable {
        public void run() {
            try {
                while (true) {
                    player.moveToPoint(targetPoint);
                    repaint();
                    Thread.sleep(15);
                }
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }

    //Thread working all the time
    private class PlayerShootRunnable implements Runnable {
        public void run() {
            try {
                while (true) {
                    //Choose which beam to shoot (depends on set mode)
                    Thread t;
                    switch (player.getBeamMode()) {
                        case 1:
                            t = new Thread(new BeamMoveRunnable(new Beam1(player.getX()+18, player.getY(), 0, -15)));
                            break;
                        case 2:
                            t = new Thread(new BeamMoveRunnable(new Beam2(player.getX()+18, player.getY(), 0, -30)));
                            break;
                        default:
                            t = null;
                            break;
                    }
                    t.start();
                    Thread.sleep(200);
                }
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }

    private class BeamMoveRunnable implements Runnable {
        private Beam beam;

        public BeamMoveRunnable(Beam beam) {
            this.beam = beam;
        }

        public void run() {
            Beam beam = this.beam;
            beams.add(beam);
            try {
                while (true) {
                    if (beam.getY() <= 0) {
                        beams.remove(beam);
                        break;
                    }
                    beam.move();
                    repaint();
                    Thread.sleep(20);
                }
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
}

[its not whole code. I cut few lines which for sure arent causing problem]

Im getting such error:

Exception in thread "AWT-EventQueue-0" java.util.ConcurrentModificationException
    at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:819)
    at java.util.ArrayList$Itr.next(ArrayList.java:791)
    at spacecommander.MainPanel.paintComponent(MainPanel.java:53)
    at javax.swing.JComponent.paint(JComponent.java:1054)

and so on...

Where lies the problem? I know what ConcurrentModificationException means but I dont know where can be the problem here.. Maybe I should make some synchronization. If yes please show where

btw, your PlayerShootRunnable loops forever. Every 200 msec, it creates a new BeamMoveRunnable thread, which also runs forever. You're creating 5 new threads every second, and none of them ever exit. — dashrb, Jan 08 '13 at 15:15
When any beam gets to 0 or bigger position loop breaks so thread ends. — Michał Tabor, Jan 08 '13 at 15:18
yes, fair point. sorry, I missed that. Still seems like substantial thread churn, IMHO. — dashrb, Jan 08 '13 at 16:07

score 1 · Answer 1 · answered Jan 08 '13 at 15:15

1

You are modifying the list of beams in another thread e.g. BeamMoveRunnable while you are iterating over the collection.

Creating a thread per object is pretty wasteful and difficult to manage.

I suggest you have a one thread which periodically calls a method on each beam to move. I suggest you not add or remove the beams while they are being drawn so you won't have to synchronize access to the collection.

answered Jan 08 '13 at 15:15

Peter Lawrey

525,659
79
751
1,130

Ok so I will change it and make one thread which would move every beam. But I think it wont help anyway because even then there is possiblity that thread will loop over collection and remove one element while paint method will try to paint it.. So how to prevent it? – Michał Tabor Jan 08 '13 at 15:21
Which is why I said, don't remove any elements. You can draw only the elements visible in the `paintComponent` instead of removing them. – Peter Lawrey Jan 08 '13 at 15:22
But wouldnt it affect performance of application? Or wouldnt throw any exception like stack overflow etc? New beam is added automaticaly every 200ms so after few minutes of game there would be many of them. – Michał Tabor Jan 08 '13 at 15:25
I see, you have a shotting game. In that case you will need to maintain a collection. I would suggest you either use the Event Thread to move the beams, or your beam updating thread gives the GUI Thread a copy of the collection, or you synchronize the collection both when iterating and when updating (which is not as good are using just the Event thread I suspect) – Peter Lawrey Jan 08 '13 at 15:37
What I made is making a copy of arrayList before drawing. Looks it works so thanks for help. – Michał Tabor Jan 08 '13 at 15:50
You have to take a copy when it is modifed. If you take a copy just before drawing you reduce the chance of a CME but it can still happen. (A copy uses an Iterator too) – Peter Lawrey Jan 08 '13 at 15:55
An alternative is to use a `CopyOnWriteArrayList` This will automatically do the copy for you as required and doesn't need to be synchronized. – Peter Lawrey Jan 08 '13 at 15:55
1

Yeah last solution is the best. I used it and doesnt see any problem (with removing as well). Thanks again – Michał Tabor Jan 08 '13 at 16:11

score 1 · Answer 2 · answered Jan 08 '13 at 15:27

1

Before iterating the beams in paintComponent create a copy of the collection and use the copy to iterate.

answered Jan 08 '13 at 15:27

StanislavL

56,971
9
68
98

Andremoniy · Answer 3 · 2019-08-29T09:02:28.543

0

You are modifying the array list beams in the thread BeamMoveRunnable (the thread which wraps this Runnable object) simultaneously with iterating it in the method paintComponent. This is the cause for the exception.

edited Aug 29 '19 at 09:02

answered Jan 08 '13 at 15:13

Andremoniy

34,031
20
135
241

Getting Concurrent Modifiraction Exception. Where is the prob.lem? (code)

3 Answers3