CopyOnWriteArrayList

Started by
13 comments, last by Code Sage 9 years, 1 month ago

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.

RIP GameDev.net: launched 2 unusably-broken forum engines in as many years, and now has ceased operating as a forum at all, happy to remain naught but an advertising platform with an attached social media presense, headed by a staff who by their own admission have no idea what their userbase wants or expects.Here's to the good times; shame they exist in the past.
Advertisement

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.

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.

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.

RIP GameDev.net: launched 2 unusably-broken forum engines in as many years, and now has ceased operating as a forum at all, happy to remain naught but an advertising platform with an attached social media presense, headed by a staff who by their own admission have no idea what their userbase wants or expects.Here's to the good times; shame they exist in the past.

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.

This topic is closed to new replies.

Advertisement