Server Chat Program -- Thread Issue

[Kody Wright]

Hello guys,

I'm currently having an issue when trying to build a server that allows for different clients to connect and
send messages to eachother.

What I'm wanting the code to do:
When a new client connects to the server (accept() call on line 73 in Server.java) a thread is spawned for
that client and handles the input/output. If a new client connects, then another thread is spawned and
the process repeats.

Here's my problem:

Whenever a new client device thread is started it should get stuck in the while(true) loop
theoretically waiting for the client to send his next message. The problem arises when the same client sends a new message...instead of the
new message being handeled by the thread that is already running for that device, another new thread is started and the old one just
sits there taking up resources but not doing anything.

I know that I've messed up my logic somewhere...I'm just not sure exactly where. I appreciate any help that a more
experienced eye may offer.

Thanks for your time!

import java.io.IOException;
import java.net.ServerSocket;
import java.util.ArrayList;

public class Server {

    /* ****************************************************
     *                    Variables                       *
     * ****************************************************/
    // Create a new server
    private static Server server = new Server();

    private String serverStatus;
    private int connections; // The number of clients connected to the server
    private int portNumber;
    private String serverName;
    private ServerSocket serverSocket;

    // Array list used to hold user messages
    private static ArrayList<String> chat = new ArrayList<String>();

    /* ****************************************************
     *                   Constructor                      *
     * ****************************************************/
    Server() {
        // Initialize server properties
        this.serverStatus = "offline";
        this.connections = 0;
        this.serverName = null;
        this.portNumber = 0;
        this.serverSocket = null;
    }

    /* ****************************************************
     *             Server Operation Methods               *
     * ****************************************************/

    /**
     * Connects the server's socket to the specified port.
     * 
     * @param int portNumber - Port that server will connect to.
     */
    public void connectServerToPort(int portNumber) {
        try {
            this.portNumber = portNumber;
            this.serverSocket = new ServerSocket(portNumber);
        } catch (IOException e) {
            ServerGUI.setMessage("\nCould not listen on port: " + portNumber);
            ServerGUI.setStatus(1);
            return;
        }
    }

    /**
     * Creates the server connection at specified port number. Loops continuously on .accept() waiting for client
     * connections. A new thread is created for each client that connects. If an exception is thrown, the server will
     * close its port connection.
     * 
     * @throws IOException
     */
    public void waitForClient() throws IOException {
        while (true) {
            ClientDevice c;
            try {
                // Returns a client connection 
                c = new ClientDevice(this.serverSocket.accept());
                // Increase connection count by one
                this.addConnection();
                ServerGUI.updateStringConnections(server);
                // Start the thread and loop around for next connection
                new Thread(c).start();
            } catch (IOException e) {
                this.stopServer();
                ServerGUI.setMessage("\nServer closed on port: " + this.portNumber);
                ServerGUI.setStatus(0);
                this.setServerStatus("offline");
                return;
            }
        }
    }

    /**
     * Attempts to close server's socket connection.
     */
    protected void stopServer() {
        try {
            this.serverSocket.close();
            this.resetConnections();
            ServerGUI.updateStringConnections(this);
        } catch (IOException e) {
            ServerGUI.setMessage("\nCould not close socket");
            return;
        }
    }

    /**
     * Checks to see if server is currently online.
     * 
     * @return boolean - Boolean determining if server is online.
     */
    public boolean isOnline() {
        if (this.serverStatus.equalsIgnoreCase("online")) {
            return true;
        } else {
            return false;
        }
    }

    /**
     * Sets the server's status (Online or Offline).
     * 
     * @param String
     *            status - New status for server.
     */
    public void setServerStatus(String status) {
        if (status.equalsIgnoreCase("online")) {
            this.serverStatus = status;
        } else if (status.equalsIgnoreCase("offline")) {
            this.serverStatus = status;
        }
    }

    /* ****************************************************
     *                Connection Methods                  *
     * ****************************************************/

    /**
     * @return An integer with the number of current connections.
     */
    public int getConnections() {
        return this.connections;
    }

    /**
     * Increases server connections by one.
     */
    public void addConnection() {
        this.connections += 1;
    }

    /**
     * Decreases server connections by one.
     */
    public void subtractConnection() {
        this.connections -= 1;
    }

    /**
     * Resets the number of current connections to zero.
     */
    public void resetConnections() {
        this.connections = 0;
    }

    /* ****************************************************
     *                Chat Get/Send Methods               *
     * ****************************************************/

    public static void addToChat(String newMessage) {
        chat.add(newMessage);
    }

    public static String readFromChat() {
        int messageIndex = chat.size();
        return chat.get(messageIndex - 1);
    }

    /* ****************************************************
     *                Getters and Setters                 *
     * ****************************************************/

    /**
     * This method allows other classes to access the current server object.
     * 
     * @return The current server object.
     */
    public static Server getServer() {
        return server;
    }

    /**
     * @return An integer with the server's port number.
     */
    public int getPortNumber() {
        return this.portNumber;
    }

    /**
     * Sets the server's port number.
     * 
     * @param int portNumber - The server's port number.
     */
    public void setPortNumber(int portNumber) {
        this.portNumber = portNumber;
    }

    /**
     * @return A String with the server's name.
     */
    public String getServerName() {
        return this.serverName;
    }

    /**
     * Sets the server's name.
     * 
     * @param String
     *            - the server's name.
     */
    public void setServerName(String serverName) {
        this.serverName = serverName;
    }
}

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.Socket;

class ClientDevice implements Runnable {

    /* ****************************************************
     *                    Variables                       *
     * ****************************************************/
    private Socket client;

    /* ****************************************************
     *                   Constructor                      *
     * ****************************************************/
    ClientDevice(Socket client) {
        this.client = client;
    }

    /**
     * Creates the buffered input and output that is used to send and receive text from the client.
     * 
     * @throws IOException
     */
    public void run() {
        BufferedReader in = null;
        PrintWriter out = null;
        try {
            in = new BufferedReader(new InputStreamReader(client.getInputStream()));
            out = new PrintWriter(client.getOutputStream(), true);
        } catch (IOException e) {
            ServerGUI.setMessage("\nError: Could not communicate with client");
            return;
        }

        while (true) {
            try {
                // Add the new message to end of array list if user is sending a message
                Server.addToChat(in.readLine());
                // Echo most recent message in array list to all clients
                out.println(Server.readFromChat());
            } catch (IOException e) {
                Server.getServer().subtractConnection();
                ServerGUI.updateStringConnections(Server.getServer());
                return;
            }
        }
    }
}

[Jayesh A Lalwani]

Do you have the client code? Is the client making a new connection everytime it sends a message? Looking at your code, it seems to me like the problem will happen when client opens a new connection for every message

[Kody Wright]

Jayesh A Lalwani wrote:Do you have the client code? Is the client making a new connection everytime it sends a message? Looking at your code, it seems to me like the problem will happen when client opens a new connection for every message

You were right on, it was flawed client logic...not server. Thanks for the insightful reply!

[Jayesh A Lalwani]

Now that your problem is solved, a few suggestions

Consider using a thread pool instead of starting threads for each client. Threads are heavy. It's better to keep them running. Also thread pool allows you to put an upper limit on how many clients can be processing simultaneously.

Rather than rely on ioexception to detect end of client session, use some sort of handshake. The client can say bye, and then you close connection. This will make resource usage less. Cleanly closing a socket connection is much lighter than relying on abrupt disconnects