0

I am working on a pretty basic server that accepts clients, and will write out data from one client to all of the other clients connected. Right now it works fine until one of the clients disconnects, at which point the next time the server attempts to iterate over the clients list, I get a concurentmodification exception out of the handle method. The way I understood the synchronized(clients) keyword was that once a thread called handle and began iterating over the client list, any thread that attempted to call add/remove on that list would block until handle was finished.

private void addClientThread(Socket _socket)
{
    System.out.println("[DEBUG]: Adding client: "+_socket.getPort());
    serverThread temp = new serverThread(this,_socket);

    try
    {
        System.out.println("Server accepted new client: " + _socket+".  Spawning new handler thread");
        temp.open();
        synchronized(clients)
        {
            clients.add(temp);
        }
        temp.start();
    }
    catch(IOException ioe)
    {
        System.out.println("Error opening thread: " + ioe);
    }
}

public void remove(serverThread threadToRemove)
{
    synchronized(clients)
    {
        clients.remove(threadToRemove);
    }
    try
    {
        threadToRemove.close();
        System.out.println("Server closed client handler thread: "+threadToRemove.getId());
    }
    catch(IOException ex)
    {
        System.out.println("IO Exception while closing thread: "+ex);
    }
}

public void handle(serverThread self,byte input)
{
    synchronized(clients)
    {
        // Pass anything the client wrote to all of the other clients
        for(serverThread temp : clients)
        {
            if(!temp.equals(self))
                temp.send(input);
        }
    }
}
JDC5011
  • 11
  • 2
  • Which line throws the exception? – Hovercraft Full Of Eels Aug 07 '16 at 03:06
  • 2
    If asking about an exception -- **always** show the complete exception stacktrace and always indicate the lines mentioned in the stacktrace. – Hovercraft Full Of Eels Aug 07 '16 at 03:08
  • for(serverThread temp : clients) – JDC5011 Aug 07 '16 at 03:09
  • I can't find the bug with this snippet, however I would recommend using a thread-safe collection instead. This is likely to be more efficient and robust than anything you're going to write yourself – Dici Aug 07 '16 at 03:19
  • Exception in thread "Thread-4" java.util.ConcurrentModificationException at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901) at java.util.ArrayList$Itr.next(ArrayList.java:851) at server.handle(server.java:148) at server.serverThread.run(serverThread.java:56) – JDC5011 Aug 07 '16 at 03:23
  • clients is a Collections.synchronizedList but from reading up on the syncrhonizedList collection, I understood that I still needed to synchronize manually when iterating over it. – JDC5011 Aug 07 '16 at 03:25
  • `Collections.synchronizedList` provides a naive and inefficient synchronization. I was talking about real synchronized collections (for example, `LinkedBlockingQueue`) not mere synchronized wrappers. – Dici Aug 07 '16 at 03:33
  • Rgr. I think I figured out where my problem was. In serverThread.send I am catching IOException in the event of a closed client connection. When my client closed, I was not detecting it until I called send, at which point, the serverThread caught the exception and called server.remove. This brings on another question though, why didn't that deadlock since remove should have to wait for access to the clients list. – JDC5011 Aug 07 '16 at 03:45

0 Answers0