12
\$\begingroup\$

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?

\$\endgroup\$
1
  • \$\begingroup\$ NEVER catch exceptions and eat them silently, as you are doing: catch(IOException e){} At least put e.printStackTrace(); between the braces. \$\endgroup\$
    – Jesper
    Commented Jan 19, 2012 at 15:18

3 Answers 3

8
\$\begingroup\$

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.

\$\endgroup\$
0
6
\$\begingroup\$

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)

\$\endgroup\$
3
  • \$\begingroup\$ Thankx for the valuable suggetion i am new to this, i am implementing Executerservice as ExecutorService es = newCachedThreadPool(); es.execute(new Double(socket)); in place of new Thread(new Double(socket)).start(); is this okay! \$\endgroup\$
    – Rex
    Commented Jan 9, 2012 at 5:42
  • \$\begingroup\$ @Rex, yes, the cached thread pool creates a number of initial threads and then recycles them when there are new tasks to execute. If you want to specify a fixed number of threads to use you can use newFixedThreadPool(); \$\endgroup\$
    – Tudor
    Commented Jan 9, 2012 at 8:33
  • 1
    \$\begingroup\$ @Tudor newCachedThreadPool will actually only create threads on demand. It will start with 0 threads. \$\endgroup\$
    – John V.
    Commented Jan 9, 2012 at 17:33
6
\$\begingroup\$

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.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.