1

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...

Improve this question
10
  • edited the code. its a while loop set as true. Commented Jun 3, 2016 at 7:03
  • Are you sure your that the output from your application going to stdout/stderr is visible to you? You see, just printing exceptions somewhere is not exactly a very robust way to deal with them. Commented Jun 3, 2016 at 7:03
  • @Jägermeister- yes i know but this is the main thread and if this exits the whole application should stop which does not happen. the client threads created continue to work seamlessly its just that the new incoming connections are not accepted any more. Commented Jun 3, 2016 at 7:09
  • @Jayesh Tripathi. Application won't stop if there's other threads(not daemon) running even if main thread stopped. Btw, your System.exit in the last line doesn't always get called. Commented Jun 3, 2016 at 7:19
  • Unless the client threads remove themselves from the array there isn't much point in even having it, and if you do have it you must sychronize all access to it. You could be getting a ConcurrentModificationException for example, which is a runtime exception. When your application stops accepting, (a) is it still running, and (b) does netstat -nap tcp still show the port as LISTENING? I would also like to know exactly what the constructor of ServerThread does. If it does any I/O at all, even constructing object streams, it shouldn't. Commented Jun 3, 2016 at 7:21

3 Answers 3

Reset to default
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.

  1. 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.

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

1. yes it may case OutOfMemoryError but that should stop the whole application.. in this case the application is running, the spawned threads are running its jjust the application which is refusing any new connections after some time. NB: have checked the firewall 2. while (m_bRunThread) {....} the loop is made false at every exception handling and termination point. double checked it.
0

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.

Improve this answer

3 Comments

the m_bRunThread loop was added to act as a fail safe for the main thread while loop while(ServerOn) at the end of the run method i check if(!ServerOn){m_bRunThread=false; conn.close;} . but still i have to ask would that solve the problem. netstat -nap tcp result is awaited as it hasn't stopped yet since yesterday.
It doesn't make any sense to have it there, so you should remove it. As you presently have it, these threads will spin mindlessly forever after the client disconnects, which is a waste of CPU, and never exit, which is a resource leak.
@EJP- the application stopped accepting connections today and the netstat -nap tcp shows the port as LISTENING
0

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
    }
}
Improve this answer

4 Comments

He has a thread per connection, not per request, and this is a normal pattern. Don't use code formatting for text that isn't code.
@EJP,,Actually I am new here but I got your point and will take care, As per what I am thinking that for every conn we are creating a new thread always, SO I am not sure whether its a good idea. Thats why I suggested not to create new thread always instead use thread pool.
But what you actually said is a new thread per request, and it isn't correct. If that doesn't reflect your meaning you should edit your answer.
@EJP,,thanks for pointing it out, I have edited the answer to make it more clear.

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.