TCP Chat (Server/Client)

I'm looking for some hints or advice regarding efficiency, performance and some good coding practices. Also I'm curious about synchronization. The server is multithreaded, so I think some operations should be synchronized.

HttpServer.java

import java.io.IOException;
import java.io.InputStream;
import java.util.Properties;

public class HttpServer {

    public static void main(String args[]) {
        int serverPort = 0;

        try (InputStream is = ClassLoader.getSystemClassLoader().getResourceAsStream("properties.xml")) {
            Properties prop = new Properties();
            prop.loadFromXML(is);
            serverPort = Integer.parseInt(prop.getProperty("serverPort"));
        }
        catch (IOException e) {
            e.printStackTrace();
        }

        ServerListener server = new ServerListener(serverPort);
        server.startServer();
    }
}

ServerListener.java

```
import java.io.IOException;
import java.net.InetSocketAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.net.SocketException;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.TreeSet;

class ServerListener implements Runnable {
Thread t;
ServerSocket serverSocket;
HashSet threadList;
LinkedList lastMessages;
TreeSet userList;
int serverPort;

ServerListener(int serverPort) {
t = new Thread(this);
threadList = new HashSet<>();
lastMessages = new LinkedList<>();
userList = new TreeSet<>();
this.serverPort = serverPort;
}

public void run() {
try {
serverSocket = new ServerSocket();
serverSocket.setReuseAddress(true);
serverSocket.bind(new InetSocketAddress(serverPort));
System.out.println("Listening on " + serverSocket.getInetAddress() + ":" + serverSocket.getLocalPort());

while (true) {
Socket socket = serverSocket.accept();
System.out.println("Client

A few of the class names are unclear.

-
ServerListener sounds like it listens for some action by the server. Instead, it is the code that runs on the server listening for connections from clients.

-
Authentication doesn't do any authentication, it is just a user's login credentials.

Most of your classes are Runnable, but they encapsulate the thread that they run in. This is a bad practice because it forces you to use one thread for each task. If the server needs to handle 100 clients, you don't want to have 100 threads. A Runnable should just be concerned with operation is executed. The code the instantiates the Runnable should be able to decide how the operation is executed.

ServerThread starts the encapsulated thread inside of the constructor. This is a step further in the wrong direction. Now, not only are you not in control of how the operation is being executed, but you are not allowed to do anything with the instance without the thread running in the background.

You read in the properties file in three different locations and throw it away when you are done. You should read it in once at the beginning and retain values. ServerThread shouldn't know where it's configuration properties are stored.

You are storing the user names an passwords in plain text in the configuration file. You are sending the user name and password in plain text over the network. Both of these are large security holes.

Be consistent with your access modifiers. Some of your classes are public, some aren't. Some of the instance variables are private, some aren't.

ServerThread.out might not be initialized when the finally block in run() executes. The way you handle threads is to primarily just print something to the console. However, a number of them imply that nothing will ever work and continuing on will just cause more problems.

StackExchange Code Review Q#78230, answer score: 6