0

I have created a Server with ServerSocket and any connection made is put into a List of Connection. The list is synchronized (Collections.synchronizedList(new ArrayList<Connection>())), and I access it properly such as:

synchronized(getConnections()) {
    Iterator<Connection> it = getConnections().iterator();
    while(it.hasNext()) {
        try {
            if(!it.next().keepAlive()) {
                it.remove();
            }
        }catch(Exception error) {
            ErrorLogger.error(error);
        }
    }
}

BUT it locks up somewhere at random times. Once it locks, it remains locked forever.

I have created a logging system which logs a message before and after every synchronized block used for this list. (COMPLETE means it went through the synchronized block). Pastebin of the logs with descriptions.

Here is my code for the ADDED part seen in the logs:

DebugFile.write("ADDED connection");
//This is used to dump empty connections
synchronized(getConnections()) {//Synchronized
    getConnections().add(mini);//Adding to List
    Iterator<Connection> it = getConnections().iterator();
    while(it.hasNext()) {
        Connection con = it.next();
        if(con.getSocket() == null || !con.getSocket().isConnected() ||
                (con.getBungeeServer().equals("Unknown") && System.currentTimeMillis() > con.getCreateTime()+30000)){
            System.out.println("[MoltresBungee] Connection disconnecting because they have not identified within 30 seconds.");
            con.stop(false);
            it.remove();//Removes expired sessions
        }
    }
    Bungee.instance.getLogger().info("MoltresBungee - Active connections: " + getConnections().size());
}
DebugFile.write("ADDED connection - COMPLETE");//Logs complete

Here is code for REMOVE ALL:

DebugFile.write("REMOVE ALL : " + name);
synchronized(getConnections()) {
    Iterator<Connection> it = getConnections().iterator();
    while(it.hasNext()) {
        Connection con = it.next();
        if(con == null || con.getSocket() == null || con.getSocket().isClosed() || con.getBungeeServer().equals(name)){
            DebugFile.write("REMOVE ALL : " + name + " removing: " + con.getBungeeServer());
            if(con != null && con.getSocket() != null) {
                try{
                    con.getSocket().close();
                }catch(Exception e){ErrorLogger.error(e);}
            }
            it.remove();
        }
    }
}
DebugFile.write("REMOVE ALL : " + name + " - COMPLETE");

The adding code is suppose to add a connection to this list then remove any items in this list which have lost connection or never completed the handshake.

The remove all code is suppose to remove anything from the list that has the same name or has closed already.

I have about ten more places I used the synchronized(getConnections()) like what is above.

If needed, here is getConnections() method:

public synchronized List<Connection> getConnections(){
    return connections;
}

Why is this locking, and how do I fix it?

EDIT: Also, clients attempting to connect retry over and over until connected, and it seems that all the previous tries are still there when the server turns on. This is when the list locks as well.

Forseth11
  • 1,418
  • 1
  • 12
  • 21
  • Do not add additional information as a comment, [edit] your post and add the information there. – Jim Garrison Dec 16 '17 at 06:08
  • Hi, im not sure i get it right because i can not see any precise information what is going on (or some log dump). But from your post i think you are dealing with some dead-lock. BTW i do not think so you have to use synchronized implementation of list at all. You already syncing it with synchronized block. Do you have some reason for using synchronized list? And the other question is if you have another synchronization on different resoruces? – Majlanky Dec 16 '17 at 07:49
  • It's a really bad idea to use the return value of a public method for locking. It's much easier to analyze issues if the locking code is all in one class and not strewn through the entire code base. Make the list private and implement an interface that interacts with the connections. After that's done I'd put the logging statements inside the synchronized block, but you'll most likely find the issue while refactoring before you need to do that. – Voo Dec 16 '17 at 13:07
  • I think the issue has to do with some of these synchronized list accessing a synchronized method which also interacts with the same synchronized list. I’m just trying to find exactly what part is locking. I’m going to add some test messages and give it a try. – Forseth11 Dec 17 '17 at 05:37

0 Answers0