Socket not receiving or sending message from/to server

Question

I'm implementing a Java multi threaded server that receive messages from a client and broadcast them to the other clients, but it's not working. The server receives the client messages only when the client application closes (the client socket closes). The application is divided in two modules: client and the server. The code is a little bit long, I know that is annoying to read all, but please help me to solve this problem.

Here is the GitHub application link to facilitate the reading. Please checkout the test branch.

---

The server module classes:

GameServer.java

/*
* This file contains the application core server responsible to wait and accept clients connections requests.
* */

package server;

import com.sun.istack.internal.NotNull;

import java.io.IOException;
import java.io.PrintWriter;
import java.net.InetAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.HashSet;
import java.util.Scanner;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
 * @author      Michael Pacheco <b>pacheco@decom.ufop.br</b>
 * @version     1.0
 *
 * This class is responsible to receive clients connections and send messages to them.
 * */
public class GameServer implements Runnable {
    /**
     * The port number used to start the server.
     * */
    private int port;

    /**
     * The socket used to accept clients connections.
     * */
    private ServerSocket serverSocket;

    /**
     * A {@link Logger} used to print messages for debug purpose.
     * */
    private final static Logger LOGGER = Logger.getLogger(GameServer.class.getName());

    /**
     * A hash set to store the clients sockets
     * */
    private HashSet<Socket> clientsSockets;

    private GameServer() {
        clientsSockets = new HashSet<>();
    }

    /**
     * Instantiates a new {@link GameServer} with a given port number.
     * @param port the port number used to start the server.
     * */
    public GameServer(int port) {
        this();
        this.port = port;
    }

    /**
     * Override method from Runnable. This method is called when an attempt to close the application occur.
     * */
    @Override
    public void run() {
        Scanner s = new Scanner(System.in);
        while (s.hasNext()) s.nextLine();
        shutdown();
    }

    /**
     * Start the server and listen for clients connections requests.
     * */
    public void start () {
        try {
            LOGGER.log(Level.INFO, "Trying to start the server...\n");
            serverSocket = new ServerSocket(this.port);
            final String ip = InetAddress.getLocalHost().getHostAddress();
            LOGGER.log(Level.INFO, "Server started!\n\tPort: {0}\n\t  IP: {1}\n", new Object[] {port, ip});
            LOGGER.log(Level.INFO, "Press Ctrl-D to shutdown the server!\n");
            waitForConnections();
        } catch (IOException e) {
            LOGGER.log(Level.SEVERE, "Failed to initialize the server! {0}\n", e.getMessage());
            e.printStackTrace();
        }
    }

    /**
     * Wait for clients connections requests
     * */
    private void waitForConnections() {
        new Thread(this).start();

        try {
            //noinspection InfiniteLoopStatement
            while (true) {
                Socket clientSocket = serverSocket.accept();
                LOGGER.log(Level.INFO, "New client connected! {0}\n", clientSocket);
                clientSocket.getOutputStream().write("You're now connected to the server\n".getBytes());
                clientSocket.getOutputStream().flush();
                allocateClient(clientSocket);
            }
        } catch (IOException e) {
            // No need for printing stacktrace if the serverSocket was closed by the shutdown method
            if (!serverSocket.isClosed())
                e.printStackTrace();
        }
    }

    /**
     * This method is responsible to delegate the communication with the client to the {@link ClientListener}.
     * @param clientSocket the client socket to delegate.
     * */
    private void allocateClient(@NotNull Socket clientSocket) {
        clientsSockets.add(clientSocket);
        new Thread(new ClientListener(clientSocket, this)).start();
    }

    /**
     * Shutdown the server
     * */
    private void shutdown () {
        try {
            LOGGER.log(Level.INFO, "Trying to shutdown the server...\n");

            // TODO Clear resources
            for (Socket soc : clientsSockets) removeClient(soc);

            serverSocket.close();

            LOGGER.log(Level.INFO, "Server successfully shut down!\n");
        } catch (IOException e) {
            LOGGER.log(Level.SEVERE, "Failed to shutdown the server! {0}\n", e.getMessage());
            e.printStackTrace();
        }
    }

    /**
     * Send a message to a single client.
     * @param message the message to be sent.
     * @param clientSocket the socket of the client that will receive the message
     * */
    private void sendMessage (@NotNull Object message, @NotNull Socket clientSocket) {
        try (PrintWriter writer = new PrintWriter(clientSocket.getOutputStream(), true)) {
            writer.println(message);
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    /**
     * Send a message to all clients except the given one.
     * @param message           the message to be sent.
     * @param excludedClient    the client that won't receive the message.
     * */
    void broadcast (@NotNull Object message, @NotNull Socket excludedClient) {
        for (Socket client : clientsSockets) {
            if (excludedClient == client)
                continue;
            sendMessage(message, client);
        }
    }

    /**
     * Remove the given client from server.
     * @param clientSocket the client to be removed.
     * */
    void removeClient (@NotNull Socket clientSocket) {
        try {
            clientSocket.close();
            clientsSockets.remove(clientSocket);
            LOGGER.log(Level.INFO, "Client removed! {0}\n", clientSocket);
            // TODO broadcast the client disconnection
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

ClientListener.java

package server;

import com.sun.istack.internal.NotNull;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.Socket;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
 * @author      Michael Pacheco <b>pacheco@decom.ufop.br</b>
 * @version     1.0
 *
 * This class is responsible to listen messages of a single client and send them to the server and then to the other clients.
 * */
public class ClientListener implements Runnable {
    /**
     * The socket used to communicate with the delegated client.
     * */
    private Socket clientSocket;

    /**
     * A reference to the {@link GameServer} used to call the {@link GameServer} broadcast method.
     * @see GameServer
     * */
    private GameServer server;

    /**
     * A {@link Logger} used to print messages for debug purpose.
     * */
    private final static Logger LOGGER = Logger.getLogger(ClientListener.class.getName());

    /**
     * Instantiate a new {@link ClientListener} with a given client socket.
     *
     * @param clientSocket  the socket of the delegated client.
     * @param server        the server reference used to call the broadcast method.
     * */
    ClientListener(@NotNull Socket clientSocket, @NotNull GameServer server) {
        this.clientSocket = clientSocket;
        this.server = server;
    }

    /**
     * Listen for client messages and send it to the server.
     * */
    @Override
    public void run() {
        try (BufferedReader reader = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()))) {
            String message;
            while ((message = reader.readLine()) != null) {
                // send received message to the server
                LOGGER.log(Level.INFO, "Message received!\n\t   From: {0}\n\tMessage: {1}\n",
                        new Object[]{clientSocket, message});
                server.broadcast(message, clientSocket);
            }
        } catch (IOException e) {
            if (!clientSocket.isClosed())
                e.printStackTrace();
        } finally {
            // send the client to server to be disconnected
            server.removeClient(clientSocket);
        }
    }
}

The client module classes:

GameClient.java

package client;

import java.io.IOException;
import java.net.Socket;
import java.util.logging.Level;
import java.util.logging.Logger;

public class GameClient {
    public static void main(String[] args) {
        final String serverAddress = args.length == 2 ? args[0] : "localhost";
        final int port = args.length == 2 ? Integer.parseInt(args[1]) : 5000;
        final Logger LOGGER = Logger.getLogger(GameClient.class.getName());

        try {
            Socket serverSocket = new Socket(serverAddress, port);
            LOGGER.log(Level.INFO, "Connection successful! {0}\n", serverSocket);
            new Thread(new ClientWriterThread(serverSocket)).start();
            new Thread(new ClientReaderThread(serverSocket)).start();
        } catch (IOException e) {
            LOGGER.log(Level.SEVERE,"Failed to connect with the server\n", e);
            e.printStackTrace();
        }
    }
}

ClientWriterThread.java

package client;

import com.sun.istack.internal.NotNull;

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

/**
 * @author Michael Pacheco
 * @version 1.0
 * This class is responsible to read client messages and send it to the server.
 * */
public class ClientWriterThread implements Runnable {
    /**
     * The socket used to send messages to the server.
     * */
    private Socket serverSocket;

    /**
     * Instantiate a new {@link ClientReaderThread} with a given server socket.
     * @param serverSocket the socket used to send messages to the server.
     * */
    ClientWriterThread(@NotNull Socket serverSocket) {
        this.serverSocket = serverSocket;
    }

    /**
     * Read messages typed by the client and send it to the server.
     * */
    @Override
    public void run() {
        try {Thread.sleep(1000);}
        catch (InterruptedException e) {e.printStackTrace();}

        BufferedReader keyboardReader = null;
        try (PrintWriter socketWriter = new PrintWriter(serverSocket.getOutputStream(), true)) {
            keyboardReader = new BufferedReader(new InputStreamReader(System.in));
            String line;
            while ((line = keyboardReader.readLine()) != null) socketWriter.write(line);
        } catch (IOException e) {
            if (!serverSocket.isClosed())
                e.printStackTrace();
        } finally {
            try {
                if (keyboardReader != null) keyboardReader.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }
}

ClientReaderThread.java

package client;

import com.sun.istack.internal.NotNull;

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

/**
 * @author Michael Pacheco
 * @version 1.0
 * This class is responsible to read messages sent by the server and show them to the client.
 */
public class ClientReaderThread implements Runnable {
    /**
     * The socket used to read messages sent by the server.
     */
    private Socket serverSocket;

    ClientReaderThread(@NotNull Socket serverSocket) {
        this.serverSocket = serverSocket;
    }

    /**
     * Read messages sent by the server.
     * */
    @Override
    public void run() {
        try (BufferedReader reader = new BufferedReader(new InputStreamReader(serverSocket.getInputStream()))) {
            String message;
            while ((message = reader.readLine()) != null) System.out.println(message);
        } catch (IOException e) {
            if (!serverSocket.isClosed())
                e.printStackTrace();
        }
    }
}

Answer 1

You use PrintWriter with autoflush, but you don't use println(...) or format(...). The .write() is not subject to the autoflush.

FYI, clientsSockets is not thread safe, you add/remove on distinct threads. Not necessarily the bug, but careful. Also, spinning non-daemon threads without keeping a reference is risky; always keep threads under control and don't count too much on interrupt()/InterruptedException/InterruptedIOException...

Answer 2

Be sure to PrintWriter.flush() the messages you send after each write() in ClientWriterThread.java.

As a side note, in order to make the code clearer change your serverSocket variable name to clientSocket in the appropriate classes (GameClient.java and ClientWriterThread.java).