Concurrent programming with a simple server

Question

I am creating a simple server that accepts a int and returns the value received twice:

public class MyServer{

    private static int port = 1234;

    public static void main(String args[]){
        try{
            ServerSocket serversock=new ServerSocket(port);
            while(true){
                Socket  socket=serversock.accept();
                new Thread(new MyClass(socket)).start();
            }
        }catch(IOException e){}
        }
    }

    class MyClass implements Runnable{

        Socket socket;

        public MyClass(Socket s){socket = s;}

        public void run(){
            try{
                DataInputStream in = new DataInputStream(socket.getInputStream());
                DataOutputStream out = new DataOutputStream(socket.getOutputStream());
                int x = in.readInt();
                out.write(2*x);
                socket.close();
            }
            catch(IOException e){}
    }
}

My question is about redesigning the code. Are there any weaknesses in the design? If yes, then what changes would make the code better?

Answer 1

If you really are planning on creating a thread for each incoming request just use and Executors.newCachedThreadPool(). As long as the incoming requests arent very often, this ExecutorService will create a new thread when you submit to it, but if a thread is idle and not being used, the service will reuse the thread instead of creating a new one.

Edit: Here is the example using your code.

public class MyServer{

    private static int port = 1234;


    public static void main(String args[]){
        try{
            final ExecutorService service = Executors.newCachedThreadPool();
            ServerSocket serversock=new ServerSocket(port);
            while(true){
                Socket  socket=serversock.accept();
                service.submit(new MyClass(socket));
            }
        }catch(IOException e){}
        }
    }

    class MyClass implements Runnable{

        Socket socket;

        public MyClass(Socket s){socket = s;}

        public void run(){
            try{
                DataInputStream in = new DataInputStream(socket.getInputStream());
                DataOutputStream out = new DataOutputStream(socket.getOutputStream());
                int x = in.readInt();
                out.write(2*x);
                socket.close();
            }
            catch(IOException e){}
    }
}

With this you have the possibility of thread reuse.

Answer 2

The ugly part that sticks out is that you are silently ignoring the exceptions... You should do some cleanup and close the sockets at least.

Improving a bit on the performance side, you are starting a new thread to execute a very small task, thus wasting a lot of time to start-up threads. You could instead submit the Runnables to an ExecutorService, provided that receiving a number from the client does not depend on user input (otherwise you will block the thread pool if the first few clients don't send anything)

Answer 3

In addition to Tudor's response, you also shouldn't write infinite loops - that's just silly. For servers that are intended to run for an unknown amount of time (possibly endlessly), I do

while (keepServerRunning) {   // where 'keepServerRunning' is a simple boolean variable
  ...

...and then use a shutdown hook to set the value to false which allows the server (and the JVM) to exit normally, without having to kill the task via kill -9 or a force close.

While it's theoretically possible that a service will run forever, it's just not good practice to pretend like that happens in reality. Clean up after yourself, and provide your apps a simple, clean way to exit normally whenever possible.