Removing while iterating on map

Question

Imagine a following interface:

interface IConnectionManager {
    fun connect(connection: Int)
    fun disconnect(connection: Int)
    fun disconnectAll()
}

Simple implementation might look like this:

class ConnectionManager: IConnectionManager {
    private val connections = mutableMapOf<Int, String>()
    
    override fun connect(connection: Int) {
        connections[connection] = "Connection $connection"
    }
    
    override fun disconnect(connection: Int) {
        connections.remove(connection)?.let {
            println("Closing connection $it")
        }
    }

    override fun disconnectAll() {
        connections.forEach {
            disconnect(it.key)
        }
    }
}

Now you probably see the problem. Whenever I call disconnectAll(), I get ConcurrentModificationException.

Demo

And I know and understand why (iterators). But I can't figure a way how to implement those disconnect() and disconnectAll() methods.

I have some ideas, some of them even works, but they are ugly, and might cause bugs elsewhere:

In disconnectAll() make copy of connections.keys and use this. This works, but might obviously cause problem when some other thread desiced to add new connection.
Pass iterator object from disconnectAll() to new disconnect(iterator). Seems just ugly, and causes code duplication and I couldn't make it to works.
Make private closeConnection(connection: Int) which wil not remove the connection from collection, and call it from both disconnect() and disconnectAll() functions. This might actualy be the best solution, but I didn't tried it yet.

Or is there some other, more elegant Kotlin solution?

Does this answer your question? [Iterating through a Collection, avoiding ConcurrentModificationException when removing objects in a loop](https://stackoverflow.com/questions/223918/iterating-through-a-collection-avoiding-concurrentmodificationexception-when-re) — Gustavo Pagani, Jul 17 '20 at 13:19
`connections.forEach { println("Closing connection $it") }` and then `connections.clear()` — Lino, Jul 17 '20 at 13:22

Tenfour04 · Answer 1 · 2020-07-17T20:15:14.930

The quick and easy solution would be to copy keys to a list and iterate the copy:

connections.keys.toList().forEach {
    disconnect(it)
}

You could alternatively repeat your disconnect code in this method without removing them and then clear the map, but if disconnected is more complicated than the one-liner, you probably don't want to be repeating the code.

You mention the problem of another thread adding a connection, but this would be a problem no matter how you handle this. You can't simultaneously modify your collection from multiple threads. If this class needs to be thread-safe, you need to wrap each public function in a synchronized block.

score 1 · Accepted Answer · answered Jul 17 '20 at 15:30

Make private closeConnection(connection: Int) which wil not remove the connection from collection, and call it from both disconnect() and disconnectAll() functions. This might actualy be the best solution, but I didn't tried it yet.

This would strongly be my recommendation. Separate the actual disconnection logic from the logic that manages which connections are registered. Something like:

class ConnectionManager: IConnectionManager {
  private val connections = mutableMapOf<Int, String>()
  
  override fun connect(connection: Int) {
    connections[connection] = "Connection $connection"
  }
  
  override fun disconnect(connection: Int) {
    if (connection in connections) {
      closeConnection(connection)
      connections.remove(connection)
    }
  }

  override fun disconnectAll() {
    connections.keys.forEach(::closeConnection)
    connections.clear()
  }

  private fun closeConnection(connection: Int) {
    println("Closing connection $connection")
  }
}

Removing while iterating on map

2 Answers2