java threads appear not to be released

Admin 1
Viewed 139

I'm trying to implement a really simple echo-back multi-threaded server.
I used the thread pool created with newFixedThreadPool, but it looks like the number of concurrent connections is fixed at nThreads (passed into newFixedThreadPool). For example, if I set nThreads to 3, then the fourth client that connects cannot interact with the server.

This is rather weird because the documentation says "Creates a thread pool that reuses a fixed number of threads operating off a shared unbounded queue."
Since the documentation also says that "If additional tasks are submitted when all threads are active, they will wait in the queue until a thread is available." I suspect my threads are never "released" so that they never became available for reuse.

I think this could be a silly mistake, but I couldn't figure out what's wrong. Here's my client handler's run() method, which I think is very similar to the code found here (client below is just a Socket connected to the client):

@Override
public void run() {
    try(BufferedReader in = new BufferedReader(new InputStreamReader(client.getInputStream()));
        BufferedWriter out = new BufferedWriter(new OutputStreamWriter(client.getOutputStream()))) {

        String input;
        while(true) {
            input = readLine();
            if(input == null)
                break;

            out.write(input);
            out.newLine();
            out.flush();
        }
    } catch(IOException e) {
        System.err.println(e, "Error from socket IO.");
        System.exit(1);
    }

    try {
        client.close();
    } catch(IOException e) {
        System.err.println(e, "Error when closing socket.");
        System.exit(1);
    }
    System.out.println("Client " + client.getInetAddress().getHostAddress() +
                        " closed connection.\n");
}

EDIT 1: I guess I should clarify that clients will immediately disconnect after receiving the response. So the threads should become available very soon.

EDIT 2: Here's the code that handles connection (listen is a ServerSocket):

while(true) {
    Socket client = null;
    try {
        client = listen.accept();
    } catch(IOException e) {
        System.err.println(e, "Error when waiting for connection.");
        System.exit(1);
    }
    System.out.println("New connection from client: " +
            client.getInetAddress().getHostAddress() + "\n"
    );

    threadPool.execute(new ConnectionHandler(client));
}

This is the thread pool initialization (in the constructor): this.threadPool = Executors.newCachedThreadPool();.

EDIT 3: Log messages (when nThread = 3, so the fourth connection is never really handled):

New connection from client: 127.0.0.1

Client 127.0.0.1 closed connection.

New connection from client: 127.0.0.1

Client 127.0.0.1 closed connection.

New connection from client: 127.0.0.1

Client 127.0.0.1 closed connection.

New connection from client: 127.0.0.1
multithreading java
2 Answers

I don't have an answer to why the forth connection is hanging, other than to say here is a version of the code which does work...

public class Test {
    public static void main(String[] args) throws Exception {
        ServerSocket listen = new ServerSocket(9999);
        ExecutorService threadPool = Executors.newFixedThreadPool(3);
        while(true) {
            Socket client = null;
            try {
                client = listen.accept();
            } catch(IOException e) {
                System.err.println(e);
            }
            System.out.println("New connection from client: " +
                    client.getInetAddress().getHostAddress() + "\n"
            );

            threadPool.execute(new ConnectionHandler(client));
        }
    }
}

class ConnectionHandler implements Runnable {
    private Socket client;

    public ConnectionHandler(Socket client) {
        this.client = client;
    }

    @Override
    public void run() {
        try(BufferedReader in = new BufferedReader(new InputStreamReader(client.getInputStream()));
            BufferedWriter out = new BufferedWriter(new OutputStreamWriter(client.getOutputStream()))) {

            String input;
            while(true) {
                input = in.readLine();
                if(input == null)
                    break;

                out.write(input);
                out.newLine();
                out.flush();
            }
        } catch(IOException e) {
            System.err.println("Error from socket IO.");
        }

        try {
            client.close();
        } catch(IOException e) {
            System.err.println("Error when closing socket.");
        }
        System.out.println("Client " + client.getInetAddress().getHostAddress() +
                " closed connection.\n");
    }
}

I connected with telnet, echoed some characters and disconnected. I did this 5 times. 

    New connection from client: 0:0:0:0:0:0:0:1

    Client 0:0:0:0:0:0:0:1 closed connection.

    New connection from client: 0:0:0:0:0:0:0:1

    Client 0:0:0:0:0:0:0:1 closed connection.

    New connection from client: 0:0:0:0:0:0:0:1

    Client 0:0:0:0:0:0:0:1 closed connection.

    New connection from client: 0:0:0:0:0:0:0:1

    Client 0:0:0:0:0:0:0:1 closed connection.

    New connection from client: 0:0:0:0:0:0:0:1

    Client 0:0:0:0:0:0:0:1 closed connection.

The only difference is the code in the question has the line...

input = readLine();

where as I needed to change it to

input = in.readLine();

to be valid. I'm wondering if in the code which hasn't been posted to the question contains a static variable or some other kind of shenanigans.

user_17646930

I just tried to reproduce my problem, and I found out the issue (and it's indeed very silly).

On my server side I want to be able to shut down the server when "exit" is entered in the console, so I created a InputHandler class (which implements Runnable) to handle server side input. It looks like this: 

class InputHandler implements Runnable {
    private Server server;

    InputHandler(Server server) {
        this.server = server;
    }

    @Override
    public void run() {
        String input = System.console().readLine();
        if(input.toLowerCase().equals("exit"))
            server.shutdown();
        else
            System.err.println("Invalid command. To exit enter \"exit\".");
    }
}

and my server's run() actually looks like this:

private void loop() {
    while(true) {
        threadPool.execute(new InputHandler(this));

        Socket client = null;
        try {
            client = listen.accept();
        } catch(Exception e) {
            System.err.println("Error when waiting for connection.");
            System.exit(1);
        }
        System.out.println("New connection from client: " +
            client.getInetAddress().getHostAddress() + "\n"
        );

        threadPool.execute(new Handler(client));
    }
}

Basically a new InputHandler thread is created in each iteration...(threadPool.execute(new InputHandler(this));). Moving this code up one line will fix it...

I didn't mention InputHandler before because it appeared to be irrelevant to me. Sadly in the end it turned out to be it.

I'm very sorry I wasted you guys' time. My apologies. Feel free to downvote if you wish : ) For now I will accept slipperyseal's answer. Thank you again for all the help.

Admin1
Related