java socket server program not accepting connections after some time

Question

I have a socket client application which listens for incoming connections in the main thread on a specified port,accepts incoming connections and starts a user thread for each connection.

this setup works for somedays and then the applicaiotns stops accepting any new new connections. the only way to recover is to restart the application. I have no clue why this happens... here is my main which accpets and starts a new thread for each connection.

while(ServerOn)
   {
        ServerSocket myServerSocket;
        private static ArrayList<Socket> connecitons; 
    try {
        // Accept incoming connections.
        Socket conn = myServerSocket.accept();
        // adds the new connections to the socket
        connections.add(conn);

        // accept() will block until a client connects to the server.
        // If execution reaches this point, then it means that a client
        // socket has been accepted.

        // For each client, we will start a service thread to
        // service the client requests.

        // Start a Service thread

        ServerThread cliThread = new ServerThread(conn);
        cliThread.start();
        } catch (IOException ioe) {
            System.out.println("Exception encountered on accept. Ignoring. Stack Trace :");
            ioe.printStackTrace();
        }
    }
    try {
        myServerSocket.close();
        System.out.println("Server Stopped");
    } catch (Exception ioe) {
        System.out.println("Problem stopping server socket");
        System.exit(-1);
    }
    }

please help.

EDIT 1

here is the class declaration:

class ServerThread extends Thread {
    Socket conn;
    boolean m_bRunThread = true;
    InputStream in = null;
    OutputStream outStream = null;



    //calling the 1-argument constructor with the socket parameters
    ServerThread(Socket s) {
        conn = s;
    }

    //Subclasses of Thread should override this method.
    public void run() {
    //lot of variables declared here.

    try {
            // Class InputStream is used for receiving data (as bytes) from client. 
            in = conn.getInputStream();
            // Class OutputStream is used for sending data (as bytes) to client.
            outStream = conn.getOutputStream();
            /*
             * 1. Go to Read Thread 
             * 2. Check for incoming data stream from Client 
             * 3. Go to read routine and process only if the data is received from the client 
             * 4. If there is no data for X minutes then close the socket.
             */
            conn.setSoTimeout(time in milliseconds);
            String inLine=null;

            while (m_bRunThread) {
                // read incoming stream
                while ((c=in.read(rec_data_in_byte))!= -1) 

                {...............

and the run method continues...

Answer 1

My observations are on the basis of the above code which you have provided. I am seeing below things which are little doubtful to me:

1. private static ArrayList<Socket> connecitons;

I am not getting the need of this list. As per the code, we are always adding the conn objects in this list for every request. We are not removing them anywhere(I have not seen full code). If this is the case then this list is always growing in size. And It may cause OutOfMemoryError.

2. while (m_bRunThread) {....}

In the run() method of ServerThread class, Is the variable m_bRunThread always true. If this is the case then this thread will never get terminated. So the no. of live threads keeps on increasing. It will make the application un-responsive. For ConcurrentModificationException: As you are not using Iterator (or for-each loop) to iterate on list, so this exception will never be thrown even in case of multi-threaded scenario.

Answer 2

The outer loop on m_bRunThread should be removed. You only need the inner read loop, and it should close the socket when it terminates or gets an exception.

Answer 3

One more thing I have observed. As per the provided code, for every incoming connection, you are creating a new service thread. But I don't think that every time to create a new thread is good idea.

Because suppose If there are 1000 connections/second, it means we are creating new 1000 threads per second, so It means its a big overhead to manage 1000 threads.

The application may become non-responsive due to the overhead of context-switches of threads. Secondly there will be frequent GC(garbage collector) cycles, which again sometimes may result into STW(stop the world) kind of situation.

So my advice is:

You can use some kind of thread pool, may be Executor framework and we can create our own ThreadPoolExecutor by providing the parameters(such as core pool size, maximum pool size, keep alive time for an idle thread etc). So now you don't have to manage the creation and lifecycle of threads. Now the idle threads can be re-used. Your application will be more responsive as the chances of frequent GCs will be less.

Even you can go for Executor's framework in-build fixed sized thread pool, implementation, with this you don't need to manage the rest overhead, except one thing that you have to pass the total no. of threads in parameter, which you want to be there to serve the requests.

Below is the sample code:

        private Executor executor = Executors.newFixedThreadPool(10);
        private ArrayList<Socket> connecitons = new ArrayList<>();
        while(ServerOn)
        {
            ServerSocket myServerSocket;

            try {
                // Accept incoming connections.
                Socket conn = myServerSocket.accept();
                connections.add(conn);

                executor.execute(new ServerThreadRunnable(conn));
            } catch (IOException ioe) {
                System.out.println("Exception encountered on accept. Ignoring. Stack Trace :");
                ioe.printStackTrace();
            }
        }
        try {
            myServerSocket.close();
            System.out.println("Server Stopped");
        } catch (Exception ioe) {
            System.out.println("Problem stopping server socket");
            System.exit(-1);
        }

Below is the task implementation..

class ServerThreadRunnable implements Runnable {
    private Socket conn;
    ServerThread(Socket s) {
        conn = s;
    }
    public void run() {
        //Here the implementation goes as per your requirement
    }
}