Search code examples
javamultithreadingconcurrencythread-safetyconcurrentmodification

Unexpected ConcurrentModificationException


Not sure why I'm getting this exception. The issue is that sometimes onClose is called prior to closeAllOpenSessions. onClose operates normally, removing the specified element from _openSessions. However, when closeAllSessions is called a ConcurrentModificationException is generated when we attempt to loop over _openSessions.

The exception does not occur when I comment out the remove statement. Considering how both these operations are protected by the same mutex, I am unsure why this is happening. What am I missing?

class B
{
    protected void onClose(Session session, List<WebSocketSessionStateful> openSessions) throws IOException
    {

        Iterator<WebSocketSessionStateful> openSessionsElements = openSessions.iterator();
        while(openSessionsElements.hasNext())
        {
            WebSocketSessionStateful openSession = openSessionsElements.next();
            if(session.equals(openSession.getSession()))
            {
                openSessionsElements.remove(); // comment me out to not throw exception
                return;
            }
        }
    }

    protected static void closeAllOpenSessions(List<WebSocketSessionStateful> openSessions) throws IOException
    {

        for(WebSocketSessionStateful openSession : openSessions) // exception generated here
        {
            openSession.getSession().close(new CloseReason(CloseReason.CloseCodes.GOING_AWAY, ""));
        }
    }
}

class A extends B
{
    private static final Object _endpointOperationMutex = new Object();

    private static final List<WebSocketSessionStateful> _openSessions = new ArrayList<WebSocketSessionStateful>();

    public void onClose(Session session, EndpointConfig config) throws IOException
    {
        synchronized (_endpointOperationMutex)
        {
            super.onClose(session, _openSessions);
        }
    }

    static void closeSessions() throws IOException
    {
        synchronized (_endpointOperationMutex)
        {
            WebSocketEndpointBase.closeAllOpenSessions(_openSessions);
        }
    }

}

Solution

  • In your closeAllOpenSesions() method, you're iterating over your openSessions collection and, for each element in it, calling close(), which of course causes your onClose() method to fire. You're then also iterating over and removing from openSessions with each call to that onClose() method - which is where your concurrent modification is coming from.

    You can conceptualize the problem more clearly as follows:

    Iterator outerLoop = coll.iterator(); 
    while (outerLoop.hasNext) { 
        Iterator innerLoop = coll.iterator(); 
        while (innerLoop.hasNext()){ 
            innerLoop.remove(); //ConcurrentModificationException
         }
     }
    

    This is no different than:

     for (Object o : coll)
         for (Object p : coll) 
             if (p.someBoolean()) remove(p); //ConcurrentModificationException
    

    You can't have two iterators iterating over the same collection simultaneously. If you do, a ConcurrentModificationException will be thrown if the inner loop removes an element that the outer loop is expecting to iterate over.

    At a glance, I don't see why you need to iterate over openSessions within B.onClose(). Why should B.onClose() be responsible for closing all open sessions? Shouldn't that be done in closeAllOpenSessions()?