ConcurrentModificationException on synchronized() code

I have a following spring bean (a little edited) This bean is responsible for adding/removing websocket connections to an internal connection (this.sockets). New connetions may be added/removed and are iterated each time a message needs to be sent.

As you can see every operation on the sockets field is wrapped in a synchronized() block it is private and is not used outside of this class, every use of this field is in the code i pasted below.

But still from time to time i am getting an ConcurrentModificartionException that happens in the for loop in onApplicationEvent call. This call may indeed be called by multiple threads in the application but my understanding was that if every call that actually modifies or iterates on this.socket variable is wrapped in synchronized() block on the same object then each of these calls cannot be invoked in parallel and have to "Wait" for each other.

How it is possible that ConcurrentModificationException is thrown in this code? Bear in mind that i am trying to understand the reason for exception i am getting and i am not looking for a quick fix that will just go around this issue (so answers like 'why dont you use XXX here' do not actually solve anything for me - i am trying to find out what i did wrong in my code).

The code:

public class MyClass extends TextWebSocketHandler implements ApplicationListener<AppEventMessage>{

    private static final Logger log= LoggerFactory.getLogger(MyClass.class);

    private Set<WebSocketSession> sockets=new HashSet<>();


    @Override
    public void afterConnectionEstablished(WebSocketSession session) throws Exception {
        super.afterConnectionEstablished(session);
        synchronized (this.sockets) {
            this.sockets.add(session);
        }
    }

    @Override
    public void afterConnectionClosed(WebSocketSession session, CloseStatus status) throws Exception {
        synchronized (this.sockets) {
            this.sockets.remove(session);
        }
    }

    @Override
    public void onApplicationEvent(AppEventMessage event) {
        synchronized (this.sockets) {
            for (WebSocketSession session : this.sockets) {
                try {
                    log.info("sendin");
                    session.sendMessage(new TextMessage(event.getMessage()));
                } catch (Throwable e) {
                    log.warn("error");
                }
            }
        }
    }
}

The most relevant part of stacktrace (with my comments)

java.util.ConcurrentModificationException
    at java.util.HashMap$HashIterator.nextNode(HashMap.java:1445)
    at java.util.HashMap$KeyIterator.next(HashMap.java:1469)
    at ...MyClass.onApplicationEvent(ApplicationEventWebsocketHandler.java:46) <- (line 46 is the for(WebSocketSession ...) line)
    at ...MyClass.onApplicationEvent(ApplicationEventWebsocketHandler.java:19) <- (line 19 is the "public class Myclass" line - how it is possible?)
    at org.springframework.context.event.SimpleApplicationEventMulticaster.invokeListener(SimpleApplicationEventMulticaster.java:163)

CodePudding user response：

Agree with your diagnosis so far. There is a second way to get a ConcurrentModificationException that is commonly overlooked...

Those synchronized locks are re-entrant which means the thread holding the lock in one method can access all methods. So the thread causing your CME is quite possibly a callback from within the for loop that ends up back in your connection-listener methods.

IMO, the most likely scenario is that the connection is only detected as closed when trying to sendMessage.

Resolution: There are cleverer solutions but this is one that cannot go wrong:

  // Take copy of sockets before iterating it to defend against self-induced ConcurrentModificationException
  for (WebSocketSession session : new HashSet<>(this.sockets)) {

CodePudding user response：

I don't see immediately what could cause the ConcurrentModificationException, but here is an advise that may help. instead of synchronizing on this.sockets declare in your class a
static final Object myLock = new Object();
And synchronize on that. This object will be the only one and unmodifiable. Better candidate to be a lock. Better yet, read up on classes Semaphore and Lock. They provide better synchronization and easier to maintain