• Advertisement
Sign in to follow this  

CopyOnWriteArrayList

This topic is 1050 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I am attempting to create a ComponentHandler, which extends MouseAdapter, that essentially handles all the buttons in my game. Every time a new Component is created it is put in to an ArrayList inside ComponentHandler. This ArrayList is iterated through inside mouseMoved() and mousePressed() in order to identify the component and handle what happens to it when it is clicked/hovered over. Randomly when I start up my game I get a ConcurrentModificationException inside mouseMoved(). Sometimes it can be the first time starting it other times it could be the 50th. I can't for the life of me figure out the problem, so I am considering just using a CopyOnWriteArrayList and hoping that solves the issue. Do you think that would be the optimal fix for this? Or am I completely derping out and missing something totally obvious? I also have methods in ComponentHandler that getComponent, addComponent, removeComponent, getComponentList. Currently I am only using getComponent inside and outside of ComponentHandler. If you need to know anything just let me know and I will be glad to tell you. Thanks for your time and help.

Share this post


Link to post
Share on other sites
Advertisement

When do components get added?  Or removed.   My guess offhand is that one of those is getting called while you're busy iterating over the list in a mouse move.  

Share this post


Link to post
Share on other sites

When do components get added?  Or removed.   My guess offhand is that one of those is getting called while you're busy iterating over the list in a mouse move.  

 

Components get added and removed whenever there is state change. At initialization of every state I clear the list and than it gets filled with whatever components are necessary for that state of the game. For the time being I ended up making the list CopyOnWriteArrayList and it has been working perfectly fine. It may not be a legit fix but, if it works it works I guess haha. If someone has a better solution though I am more than happy to hear it.

Edited by Code Sage

Share this post


Link to post
Share on other sites


ConcurrentModificationException inside mouseMoved().

 

This is a terrible exception name.  Not only can you modify something in two different threads, but just iterating over the collection and trying to change something can cause this.  It may not have anything to do with multiple threads.

Share this post


Link to post
Share on other sites

ConcurrentModificationException inside mouseMoved().

 
This is a terrible exception name.  Not only can you modify something in two different threads, but just iterating over the collection and trying to change something can cause this.  It may not have anything to do with multiple threads.

So is my fix sufficient? Or is the whole thing a mistake in the first place..

Share this post


Link to post
Share on other sites

 

 

ConcurrentModificationException inside mouseMoved().

 
This is a terrible exception name.  Not only can you modify something in two different threads, but just iterating over the collection and trying to change something can cause this.  It may not have anything to do with multiple threads.

So is my fix sufficient? Or is the whole thing a mistake in the first place..

 

 

If it was me I would undo the fix and see if I could find the problem.  Without seeing the code I can't tell you if this has fixed the problem or just hidden it.

Share this post


Link to post
Share on other sites

ConcurrentModificationException inside mouseMoved().

 
This is a terrible exception name.  Not only can you modify something in two different threads, but just iterating over the collection and trying to change something can cause this.  It may not have anything to do with multiple threads.

So is my fix sufficient? Or is the whole thing a mistake in the first place..
 
If it was me I would undo the fix and see if I could find the problem.  Without seeing the code I can't tell you if this has fixed the problem or just hidden it.

I could send you my code if you wouldn't mind looking over it for me. Just promise I won't see it in the coding horrors section later haha.

Share this post


Link to post
Share on other sites


I could send you my code if you wouldn't mind looking over it for me. Just promise I won't see it in the coding horrors section later haha.

 

Don't do that.  Instead, post the smallest example you can that still produces the bug.  That way others can learn from it.

Share this post


Link to post
Share on other sites


Don't do that.  Instead, post the smallest example you can that still produces the bug.  That way others can learn from it.

 

I have been trying to recreate it for a while and I simplified it down as far as possible, but now the exception is never thrown... Honestly as I keep adding more and more trying to throw the exception I will have figured out the problem by the time it happens. I'll make a post if I figure it out, but I don't think i'll post any code unless you wanna take a look at my full source.

Share this post


Link to post
Share on other sites

Are you using the for(var: collection) construct to iterate? That silently creates an Iterator, which throws ConcurrentModificationException if you modify the collection it is backed by.

 

If you are removing the element you are examining, such as below, then you should explicitly create an iterator and use its remove() method.

Iterator<Foo> iter = fooArrayList.iterator();
while( iter.hasNext() ) {
    Foo e = iter.next();

    if( e.isBar() ) {
        iter.remove();
    }
}

Alternatively, simply build up a list of elements that must be removed, and then remove them one by one after you finish iterating through the list of elements.

 

Finally, if the elements are UI components in a Swing or AWT hierarchy, and you absolutely positively cannot be sure whether you are in an event handler or not while iterating on your collection, you can make sure you don't modify the view from inside the event handler thread by using the SwingUtilities.invokeLater() method.

for(Foo e: fooCollection) {
    if( e.isBar() ) {
        SwingUtilities.invokeLater(new Runnable() {
            void run() {
                fooCollection.remove(e);
            }
        });
    }
}

Disclaimer: I do not claim that any of the above suggestions meet best practices; only that they let you avoid modifying a collection while it is being iterated over.

Share this post


Link to post
Share on other sites

 

I have been trying to recreate it for a while and I simplified it down as far as possible, but now the exception is never thrown... Honestly as I keep adding more and more trying to throw the exception I will have figured out the problem by the time it happens. I'll make a post if I figure it out, but I don't think i'll post any code unless you wanna take a look at my full source.

 

 

That is probably because ConcurrentModificationException is not guaranteed to be thrown if you modify the collection while iterating on it; it's a "best-effort" error notification, not a sure thing.

Share this post


Link to post
Share on other sites

Are you using the for(var: collection) construct to iterate? That silently creates an Iterator, which throws ConcurrentModificationException if you modify the collection it is backed by.

 

If you are removing the element you are examining, such as below, then you should explicitly create an iterator and use its remove() method.

Iterator<Foo> iter = fooArrayList.iterator();
while( iter.hasNext() ) {
    Foo e = iter.next();

    if( e.isBar() ) {
        iter.remove();
    }
}

 

I already tried this method of doing the iteration and it sadly did not fix the exception. Only time it gets fixed for me is when I CopyOnWriteArrayList :/ And as for your other method I am using custom Buttons. They are not Swing or AWT.

Share this post


Link to post
Share on other sites

I believe I have figured it out. Well at least for the past 100+ times I have opened my game it has not occurred.

 

Let me brake down the basics of what I was doing first. Inside my Game class is where I initialize all my handlers, so for instance my ComponentHandler. After my handlers are initialized I initialize the starting State of the game, in this case the MainMenuState. OK, so inside of the ComponentHandler constructor is where I add my MouseListener and MouseMotionListener to game, therefore the mouseMoved() method starts getting called before the State of the game is initialized. Well the problem doesn't exactly have to do with the order of my initializations, but it did play an important factor. This is what my mouseMoved() method looked like before the fix:

public void mouseMoved(MouseEvent e) {

     int x = e.getX();
     int y = e.getY();
     int componentId = -1;

     for (Component c : components) {

          if (c == null || c.isDisabled())
               continue;

          if (c.contains(x, y)) {
               componentId = c.getId();

               if (!c.getHovered())
                    c.setHovered(true);
          } else
               c.setHovered(false);
     }

     if (componentId != -1) {

          switch(componentId) {
               // TODO: HANDLE THINGS
          }
     }
}

So as you can see that it will start to loop through all the components in order to find the componentId of whatever the x, y coordinates of the mouse are contained within.

 

Ok so now that the ComponentHandler is initialized, its time to initialized my MainMenuState. Well inside my MainMenuState is where the Interface is built for the main menu. This includes adding Components (Buttons) to the interface. Every Button() extends the Component class and upon creation is automatically added to the List of components inside ComponentHandler. So this is where the exception is thrown, while I am already iterating through the list of components inside ComponentHandler, I am also trying to add new buttons to that list through the creation of the main menu interface. Since you can't add or remove items to a list while iterating through it a ConcurrentModificationException was thrown.

 

How I decided to fix this problem is by stopping the mouseMoved() from iterating through the List without the State of the game first being initialized like so:

public void mouseMoved(MouseEvent e) {

     int x = e.getX();
     int y = e.getY();
     int componentId = -1;

     if (State.currentState == null || !State.currentState.isInitialized()) // Fix
          return;

     for (Component c : components) {

          if (c == null || c.isDisabled())
               continue;

          if (c.contains(x, y)) {
               componentId = c.getId();

               if (!c.getHovered())
                    c.setHovered(true);
          } else
               c.setHovered(false);
     }

     if (componentId != -1) {

          switch(componentId) {
               // TODO: HANDLE THINGS
          }
     }
}

Inside the current States class I just be sure to set the initialized boolean to true at the end of the constructor. Also at the beginning of every States initialization I clear the list of components inside ComponentHandler. And just for the record I did the same thing with the mousePressed() to avoid any issues with that.

 

I am not 100% sure if this was the cause, however it seems to be fixed for now without the use of any CopyOnWriteArrayLists smile.png If you have any questions, comments or concerns let me know.

Edited by Code Sage

Share this post


Link to post
Share on other sites

Gotcha; so, what was happening was that in one thread you are modifying the collection, and in this event handler thread you are iterating over it.

 

Assuming you will ever have more than one state (which of course you will; otherwise, why have states?) your solution will work only so long as the "components" collection can only be modified (add, remove, sort, whatever) during state initialization, and as long as state initialization and that iteration loop are either synchronized on the same object or so long as state initialization is only performed in the event dispatcher thread, such as in the call stack downstream of a button being clicked or in an invokeLater() Runnable.

 

As it stands, that code will work with only one state in existence because isInitialized() will never become false again after it becomes true. Once you have another state to switch to, unless you initialize it fully before replacing the value of "currentState", it will be possible for the event handler to start walking the collection while state initialization begins in another thread.

 

 

Edit: I say all that because you appear to be using java.awt.event.MouseEvent, so I can only assume you are using the AWT Event Dispatch Thread model. If this is not true, then all my advice is unfounded, because I have no idea how your event pump is working.

Edited by Wyrframe

Share this post


Link to post
Share on other sites

Gotcha; so, what was happening was that in one thread you are modifying the collection, and in this event handler thread you are iterating over it.

 

Assuming you will ever have more than one state (which of course you will; otherwise, why have states?) your solution will work only so long as the "components" collection can only be modified (add, remove, sort, whatever) during state initialization, and as long as state initialization and that iteration loop are either synchronized on the same object or so long as state initialization is only performed in the event dispatcher thread, such as in the call stack downstream of a button being clicked or in an invokeLater() Runnable.

 

As it stands, that code will work with only one state in existence because isInitialized() will never become false again after it becomes true. Once you have another state to switch to, unless you initialize it fully before replacing the value of "currentState", it will be possible for the event handler to start walking the collection while state initialization begins in another thread.

 

 

Edit: I say all that because you appear to be using java.awt.event.MouseEvent, so I can only assume you are using the AWT Event Dispatch Thread model. If this is not true, then all my advice is unfounded, because I have no idea how your event pump is working.

 

Well when the State of the game changes it has to be done like so: (Assume we are going from the MainMenuState to the GameState by clicking the play button)

case PLAY_GAME:
     State gameState = new GameState(game);
     State.setState(gameState);
     break;

When the GameState is being constructed it calls its supers constructor first, which looks like this:

public State(final Game game) {

     if (currentState != null) {
          currentState.setInitialized(false);
          currentState = null;
     }
     game.getComponentHandler().getComponents().clear();
}

So before anything is really constructed inside of GameState the currentState is uninitialized and than set to null. This should effectively stop the mouseMoved() and mousePressed() from doing anything while GameState is adding/removing its components to the collection. At least I hope it is working like this.

Edited by Code Sage

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement