TCP Chat (Server/Client)

Question

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<ServerThread> threadList;
 LinkedList<Message> lastMessages;
 TreeSet<String> 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 connected (" + socket.getRemoteSocketAddress() + ")");
 threadList.add(new ServerThread(socket, threadList, lastMessages, userList));
 }
 }
 catch (SocketException e) {
 System.out.println("SocketException " + e);
 }
 catch (IOException e) {
 System.out.println("IOException " + e);
 }
 finally {
 try {
 serverSocket.close();
 }
 catch (IOException e) {
 System.out.println("IOException " + e);
 }
 }
 }
 void startServer() {
 t.start();
 }
 void stopServer() throws IOException {
 serverSocket.close();
 }
}

ServerThread.java

import java.io.*;
import java.net.Socket;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.Properties;
import java.util.TreeSet;
class ServerThread implements Runnable {
 private Thread t;
 private Socket socket;
 private HashSet<ServerThread> threadList;
 private LinkedList<Message> lastMessages;
 private ObjectOutputStream out;
 private Message message;
 private Authentication auth;
 private int maximumMessages;
 private TreeSet<String> userList;
 ServerThread(Socket socket, HashSet<ServerThread> threadList, LinkedList<Message> lastMessages, TreeSet<String> userList) {
 this.socket = socket;
 this.threadList = threadList;
 this.lastMessages = lastMessages;
 this.userList = userList;
 t = new Thread(this);
 t.start();
 try (InputStream is = ClassLoader.getSystemClassLoader().getResourceAsStream("properties.xml")) {
 Properties prop = new Properties();
 prop.loadFromXML(is);
 maximumMessages = Integer.parseInt(prop.getProperty("maximumLastMessages"));
 } catch (IOException e) {
 e.printStackTrace();
 }
 }
 public void run() {
 try {
 out = new ObjectOutputStream(socket.getOutputStream());
 }
 catch (IOException e) {
 System.out.println("IOException: " + e);
 }
 try (ObjectInputStream messageObject = new ObjectInputStream(socket.getInputStream())) {
 auth = (Authentication) messageObject.readObject();
 if (checkUser(auth)) {
 out.writeObject(new Message("Server", "Connected"));
 userList.add(auth.getUserName());
 for (Message msg : lastMessages) sendMessage(msg);
 while (true) {
 try {
 message = (Message) messageObject.readObject();
 }
 catch (EOFException e) {
 break;
 }
 lastMessages.addLast(message);
 if (lastMessages.size() > maximumMessages) lastMessages.removeFirst();
 for (ServerThread thread : threadList) thread.sendMessage(message);
 }
 }
 else {
 out.writeObject(new Message("Server", "Authentication failed"));
 System.out.println("Access denied (" + socket.getRemoteSocketAddress() + ")");
 }
 }
 catch (ClassNotFoundException e) {
 System.out.println("Class not found " + e);
 }
 catch (IOException e) {
 System.out.println("IOException: " + e);
 }
 finally {
 try {
 out.close();
 }
 catch (IOException e) {
 System.out.println("Cannot close connection " + e);
 }
 threadList.remove(this);
 userList.remove(auth.getUserName());
 System.out.println("Client disconnected (" + socket.getRemoteSocketAddress() + ")");
 }
 }
 private void sendMessage(Message msg) {
 try {
 out.writeObject(msg);
 }
 catch (NullPointerException e) {
 System.out.println("Cannot send message. Message is empty " + e);
 } catch (IOException e) {
 System.out.println("Cannot send message " + e);
 }
 }
 private boolean checkUser(Authentication auth) {
 boolean result = false;
 try (InputStream is = ClassLoader.getSystemClassLoader().getResourceAsStream("userlist.xml")) {
 Properties prop = new Properties();
 prop.loadFromXML(is);
 if (auth.getUserName().equals("Server") || userList.contains(auth.getUserName())) {
 result = false;
 }
 else result = prop.getProperty(auth.getUserName()).equals(auth.getUserPassword());
 }
 catch (NullPointerException e) {
 System.out.println("User not found");
 return false;
 }
 catch (IOException e) {
 System.out.println("Cannot open user list " + e);
 }
 return result;
 }
}

Message.java

import java.io.Serializable;
import java.util.Date;
class Message implements Serializable {
 private String userName;
 private String message;
 private Date date;
 Message(String userName, String message) {
 this.userName = userName;
 this.message = message;
 date = new Date();
 }
 public String toString() {
 if (userName.equals("Server")) {
 return message;
 }
 else return date + " [" + userName + "] " + message;
 }
}

Authentication.java

import java.io.Serializable;
public class Authentication implements Serializable {
 private String userName;
 private String userPassword;
 Authentication(String userName, String userPassword) {
 this.userName = userName;
 this.userPassword = userPassword;
 }
 String getUserName() {
 return userName;
 }
 String getUserPassword() {
 return userPassword;
 }
}

HttpClient.java

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.ObjectOutputStream;
import java.net.Socket;
public class HttpClient {
 public static void main(String args[]) throws IOException {
 String message;
 String userName;
 String userPassword;
 BufferedReader bReader = new BufferedReader(new InputStreamReader(System.in));
 System.out.print("Enter your name: ");
 userName = bReader.readLine();
 System.out.print("Enter your password: ");
 userPassword = bReader.readLine();
 try (Socket clientSocket = new Socket("192.168.1.123", 777)) {
 new ClientReceiver(clientSocket);
 try (ObjectOutputStream out = new ObjectOutputStream(clientSocket.getOutputStream())) {
 Authentication auth = new Authentication(userName, userPassword);
 out.writeObject(auth);
 while ((message = bReader.readLine()) != null) {
 Message messageObject = new Message(userName, message);
 out.writeObject(messageObject);
 }
 }
 }
 }
}

ClientReceiver.java

import java.io.EOFException;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.net.Socket;
import java.net.SocketException;
class ClientReceiver implements Runnable {
 private Thread t;
 private Socket clientSocket;
 private Message message;
 ClientReceiver(Socket clientSocket) {
 t = new Thread(this);
 t.start();
 this.clientSocket = clientSocket;
 }
 public void run() {
 try (ObjectInputStream in = new ObjectInputStream(clientSocket.getInputStream())) {
 while (true) {
 try {
 message = (Message) in.readObject();
 }
 catch (EOFException e) {
 break;
 }
 System.out.println(message);
 }
 }
 catch (SocketException e) {
 System.out.println("Disconnected");
 }
 catch (ClassNotFoundException e) {
 System.out.println("Class not found " + e);
 }
 catch (IOException e) {
 System.out.println("IOException: " + e);
 }
 }
}

Answer 1

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.

• Daniil Molchanov: Do you recommend using NIO with selector rather than multithreaded server? ServerSocket's accept() is blocking method, so I need to have a thread for every client.
• unholysampler: I haven't used selector, but am aware of the concept. If you are going to be handling that many clients, one thread per client does fall apart. Another option would be a hybrid model: Create a runnable that uses the selector for the connections it manages. Run multiple instances and have a mechanism for distributing the connections between the instances. -- The point I was trying to make in my answer was that when you write the class just as a runnable, you have the freedom to decide if it is run in thread, with a thread pool or even as a blocking call without changing the implementation.

Answer 2

threadlist is not thread safe, this will lead to odd thingsTM (aka race conditions) as you add and remove threads. You can wrap it in Collections.concurrentSet on creation to make it thread safe.

Same with the lastMessage list. in particular when adding you do:

lastMessages.addLast(message);
if (lastMessages.size() > maximumMessages) lastMessages.removeFirst();

This needs to be synchronized so no thread can mess with the list while another is checking the length to see if one needs to be removed.

Answer 3

It is incredibly misleading to call your classes HttpServer and HttpClient when in fact the code has nothing to do with the HTTP protocol.

• unholysampler: This is a big one that I missed in my naming section.