Threads not behaving how I want in a Java chat application

Question

I'm developing a chat application, and I'm using global variables as a way to allow server threads to access and update shared information (I know this isn't recommended, but I'm going to stick with this method for the time being).

What I want to happen, is that every time a new message is submitted, it prints to all the clients so that all the clients can see them.

What actually happens, is that it only updates the client that I've submitted a message with. Though it does kinda of receive messages from the other client. It's not simultaneous.

Where am I going wrong?

class Server {

public static void main(String argv[]) throws Exception { 

    boolean listening = true;
    int portNumber = 6000;

    try (ServerSocket serverSocket = new ServerSocket(portNumber)) { 
        while (listening) {
            new ServerThread(serverSocket.accept()).start();
        }
    } catch (IOException e) {
        System.err.println("Could not listen on port " + portNumber);
        System.exit(-1);
    }
}
}

Server thread for handling clients.

public class ServerThread extends Thread {
private Socket socket = null;
public ServerThread(Socket socket) {
    super("ServerThread");
    this.socket = socket;
}

public void run () {

    int msgCnt = 0;
    boolean running = true;

    try (
        PrintWriter out = new PrintWriter(socket.getOutputStream(), true);
        BufferedReader in = new BufferedReader(
            new InputStreamReader(
                socket.getInputStream()));
    ) {
        String inputLine, outputLine;

        while(running) {



        while ((inputLine = in.readLine()) != null) {
            if (Global.messageCount > msgCnt){
                out.println(Global.currentMessage + " received from others");
                msgCnt = msgCnt + 1;
            }
            outputLine = inputLine;
            Global.currentMessage = inputLine;
            Global.messageCount = Global.messageCount + 1;
            //out.println(outputLine);


        }
            //if (outputLine.equals("QUIT"))
            //    break;
        }
    } catch (IOException e) {
        e.printStackTrace();
    }
}
}

Also, client code

class Client {
public static void main(String argv[]) throws Exception { 
    String sentMessage;  //variable for input
    String status;
    boolean running;

    BufferedReader inFromUser = new BufferedReader(new InputStreamReader(System.in));

    Socket clientSocket = new Socket("127.0.0.1", 6000); //name of computer to connect with and port number to use

    DataOutputStream outToServer = 
            new DataOutputStream(clientSocket.getOutputStream());
    BufferedReader inFromServer = 
            new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));

    running = true;

    while(running)
    {
    sentMessage = inFromUser.readLine(); //user inputs text to variable 'sentMessage'

    outToServer.writeBytes(sentMessage + '\n'); //the variable is sent to the server

    status = inFromServer.readLine(); //the modified data is returned from the server

    System.out.println("FROM SERVER: " + status); //display to user

    }     
clientSocket.close();     
}
}

Global class, just a few variables

public class Global {
    public static String currentMessage = "Nothing";
    public static int messageCount = 0;
}

I suggest you avoid mixing text (Reader/Writer) and binary (Stream) as this is a common source of confusion. You should pick one or the other. — Peter Lawrey, Apr 23 '14 at 15:13
Where is the code where you write the same line to every client? — Peter Lawrey, Apr 23 '14 at 15:15
The line, out.println(Global.currentMessage + " received from others"); It's part of the thread so I assumed this would happen for all clients. — WorldsWorstProgrammer, Apr 23 '14 at 15:16
@Braj Ah, yes, I was changing the ports while pasting the code in, the ports are normally the same and the problem still occurs. — WorldsWorstProgrammer, Apr 23 '14 at 15:18
Need to see the `Global` class... ah maybe not, was thinking there could be some `ThreadLocal`s in there, but I guess everything is referenced statically... still, couldn't hurt to make sure nothing fishy is going on — Sean Adkinson, Apr 23 '14 at 15:22
Ok, yeah the global class is just simply 2 variables at the moment. — WorldsWorstProgrammer, Apr 23 '14 at 15:25

score 1 · Answer 1 · answered Apr 23 '14 at 15:36

I think your error is in this logic here:

    while ((inputLine = in.readLine()) != null) {
        if (Global.messageCount > msgCnt){
            out.println(Global.currentMessage + " received from others");
            msgCnt = msgCnt + 1;
        }
        outputLine = inputLine;
        Global.currentMessage = inputLine;
        Global.messageCount = Global.messageCount + 1;
    }

Notice that you are only going to begin executing the contents of this while loop once a client has submitted a message. If the thread is blocked on in.readLine() and another thread submits a message, there is nothing to cause this while loop to kick off. I would expect you would get the output you want on the next message submitted by this client.

One other note: it seems like you should set msgCnt = Global.messageCount, otherwise you could have a situation where Global.messageCount is more than +1 of the current msgCnt, and then you forevermore run the contents of your if (Global.messageCount > msgCnt) statement.

Thank you. I changed the loop, and it definitely fixed something. The clients still only receive feedback on their own messages though. Not from other clients. I'm baffled. — WorldsWorstProgrammer, Apr 23 '14 at 15:51

score 0 · Answer 2 · edited May 23 '17 at 11:57

You need to maintain the list of all the clients.

If any message is received from any client just iterate through the list and send the messages to other clients.

Sample code:

List<ServerThread> allClients=new ArrayList<ServerThread>();

class ServerThread extends Thread {
   private Socket socket = null;
   private PrintWriter out;

    public ServerThread(Socket socket) {
        ...
        allClients.add(this);
    }
   ...

       ...
       //code where you received any message
       for (ServerThread client : allClients) {
             if(client != this) {
                  client.out.println(Global.currentMessage + " received from others");
             }
       }
       ...
}

I have posted a sample code in the same context.

Please have a look at Java Server with Multiclient communication. and follow the post to find more samples.

score 0 · Answer 3 · answered Apr 23 '14 at 17:29

From Java Concurrency in Practice by Brian Goetz:

[...] in the absence of synchronization, there are a number of reasons a thread might not immediately -- or ever -- see the results of an operation in another thread. Compilers may generate instructions in a different order than the "obvious" one suggested by the source code, or store variables in registers instead of in memory; processors may execute instructions in parallel or out of order; caches may vary the order in which writes to variables are committed to main memory; and values stored in processor-local caches may not be visible to other processors. These factors can prevent a thread from seeing the most up-to-date value for a variable and can cause memory actions in other threads to appear to happen out of order -- if you don't use adequate synchronization.

Declare Global.currentMessage to be volatile. Also, use AtomicInteger for Global.messageCount, calling AtomicInteger.getAndIncrement() to increment the counter.

You will also encounter other concurrency-related issues with your design. I suggest reading up on concurrency in Java before continuing. The aforementioned book is a most excellent resource.

Threads not behaving how I want in a Java chat application

3 Answers3