Simple TCP client-server solution

Question

I have implemented a simple client-server solution using Java sockets and useful classes from java.util.concurrent.

The communication protocol is rather simple:

1. Client sends a string with the appended request_info#.
2. A server responds to a client with the message that provides a description on how to use a server.
3. If client sends server_time then a server will send back a current server time.
4. If client sends a quit - a server will process a client disconnect.
5. A default case: client sends an arbitrary string then a server simply bounces it back to a client.

I have tested my solution by spawning a fixed number of client threads. Each of them sends a command from a txt file to a server.

Some technical remarks:

1. A configuration for the client and server (such as port number and host address) is loaded from an .xml file.
2. To start a server: java –jar app.jar –server –config config.xml.
3. To start a client: java –jar app.jar –client –config config.xml.

I would like to seek any suggestions on whether I have correctly implemented the following class (serves as an abstraction for a message between client and server):

public class Message {
    private Socket socket;
    private String message;
    private static Logger logger = Logger.getLogger(Message.class);

    public void setSocket(Socket socket) {
        this.socket = socket;
    }

    public String getMessage() {
        return message;
    }

    public Socket getSocket() {
        return socket;
    }

    public void setStringMessage(String message) {
        this.message = message;
    }

    public void setMessageInputSources(InputStream inputStream) throws IOException {
        String s;
        BufferedReader bufferRead = new BufferedReader(new InputStreamReader(inputStream));
        s = bufferRead.readLine();
        this.message = s;
    }

    public synchronized String getCommand() {
        String stringMessage = this.getMessage();
        return StringUtils.upperCase(stringMessage.split("#")[0]);
    }

    public void send(String message) throws IOException{
        DataOutputStream dataOutputStream = new DataOutputStream(this.socket.getOutputStream());
        dataOutputStream.writeUTF(message);
        dataOutputStream.flush();

        this.setStringMessage(message);
    }

    public Message receive() throws IOException {
        String data;
        DataInputStream dataInputStream = new DataInputStream(this.socket.getInputStream());
        data = dataInputStream.readUTF();
        this.setStringMessage(data);
        return this;
    }

    @Override
    public String toString() {
        return this.message;
    }
}

Do I properly manage a case when all client threads are done with their jobs and I shut down an application? Are there any improvements to be made?

public class MainTest {

    private static final int NUMBER_OF_THREADS = 4;

        public static void main(String[] args) throws IOException, InterruptedException {
            CountDownLatch countDownLatch = new CountDownLatch(NUMBER_OF_THREADS);

            Thread serverThread = new Thread(() -> Server.start(new File("config.xml"), countDownLatch));
            serverThread.setName("server_thread");

            Thread clientThread = new Thread(() -> {
                try {
                    new Client(new File("config.xml"), new FileInputStream("commands.txt")).start(NUMBER_OF_THREADS, countDownLatch);

                } catch (ExecutionException | IOException | InterruptedException e) {
                    e.printStackTrace();
                }
            });
            clientThread.setName("Running client threads, " + 10);
            serverThread.start();
            clientThread.start();
            if(countDownLatch.getCount() == 0){
                System.exit(0);
            }
        }
    }

Here is the link to my project: GitHub Repository

Answer 1

Just a few things I would look into if this code will be used for any real purposes. The Client does not handle failing to connect to the Server. It assumes the Server will always be up and available. I don't see any kind of error handling if the Server/Client connection gets broken while sending messages. You should definitely add code to clean up your sockets properly after you are done with them.

Design wise I would separate the Socket connection from the actual data being sent. Maybe add connect/disconnect methods to the Client and Server to handle the Sockets and use an internal Thread that gets started after you have a valid connection. Should a data message know how to encode/decode itself? Should a data message know how to send/recv itself? In my opinion a separate Client/Server Service should be created to manage these functions. Data should be as dumb as possible. You also have a lot of Static state throughout your code I would remove that in favor of local class instance variables. Doing this will give you a lot more flexibility in the long run.

Another thing to consider is a Factory class which can create the Client and Servers. Use the Factory class to handle the configuration of the Client and Server rather than having them configure themselves. This will allow you to test the Client and Servers easier so you don't have to have a specific configuration file for testing.

Answer 2

Your MainTest file has awkward indentation. I don't understand the extra tab after private static final int NUMBER_OF_THREADS = 4;. Instead, you should keep indentation same for the same level of code:

public class MainTest {

    private static final int NUMBER_OF_THREADS = 4;

    public static void main(String[] args) throws IOException, InterruptedException {
        CountDownLatch countDownLatch = new CountDownLatch(NUMBER_OF_THREADS);

        Thread serverThread = new Thread(() -> Server.start(new File("config.xml"), countDownLatch));
        serverThread.setName("server_thread");

        Thread clientThread = new Thread(() -> {
            try {
                new Client(new File("config.xml"), new FileInputStream("commands.txt")).start(NUMBER_OF_THREADS, countDownLatch);

            } catch (ExecutionException | IOException | InterruptedException e) {
                e.printStackTrace();
            }
        });
        clientThread.setName("Running client threads, " + 10);
        serverThread.start();
        clientThread.start();
        if(countDownLatch.getCount() == 0){
            System.exit(0);
        }
    }
}

Here:

public Message receive() throws IOException {
    String data;
    DataInputStream dataInputStream = new DataInputStream(this.socket.getInputStream());
    data = dataInputStream.readUTF();
    this.setStringMessage(data);
    return this;
}

The data variable can be declared at the time of use:

public Message receive() throws IOException {
    DataInputStream dataInputStream = new DataInputStream(this.socket.getInputStream());
    String data = dataInputStream.readUTF();
    this.setStringMessage(data);
    return this;
}

In Java, you should declare variables where you use them.

Also, the data variable is useless if you immediately set the string message to data:

public Message receive() throws IOException {
    DataInputStream dataInputStream = new DataInputStream(this.socket.getInputStream());
    this.setStringMessage(dataInputStream.readUTF());
    return this;
}

Same here:

public void setMessageInputSources(InputStream inputStream) throws IOException {
    String s;
    BufferedReader bufferRead = new BufferedReader(new InputStreamReader(inputStream));
    s = bufferRead.readLine();
    this.message = s;
}

Should be:

public void setMessageInputSources(InputStream inputStream) throws IOException {
    BufferedReader bufferRead = new BufferedReader(new InputStreamReader(inputStream));
    this.message = bufferRead.readLine();
}

I am not very experienced with Threads, so I'm going to leave that to more experienced programmers.