Simple multi-threaded Java server loop I'm new to sockets and servers in general and I would like to know if I'm doing something really wrong in the following server loop for spawning threads to process requests.

private static ServerSocket server;
private static final Object request_counter_lock = new Object();
private static int request_counter = 0;
private static boolean exit = false;

public static void main(String[] args) {
    try {
        server = new ServerSocket(8000,100);
        // Loop until one of the clients sends an exit request.
        while (!exit) {
            try {
                final Socket socket = server.accept();
                // When a connection is accepted we increment the request
                // counter so when the server starts to shut down it waits
                // until all requests are finished processing.
                synchronized(request_counter_lock) { request_counter++; }
                // Fire up a new thread to process requests.
                new Thread(new Runnable() {
                    @Override
                    public void run() {
                        try {
                            processConnection(socket);
                        } catch (IOException e) {
                            logger.error("unable to process request: " + e.getMessage());
                        } finally {
                            // Make sure we close the socket.
                            SocketCloser.close(socket);
                            // Also decrement the request counter because we just closed
                            // the request socket which means we are done with this client.
                            synchronized(request_counter_lock) { request_counter--; }
                        }
                    }
                }).start(); // Fire up the thread.
            } catch (IOException e) {
                logger.error("error accepting socket connection: " + e.getMessage());
            }
        }
    } catch (IOException e) {
        logger.fatal("unable to start server: " + e.getMessage());
    } finally {
        // If the while loop breaks that means we received an exit request from
        // one of the clients but it's possible that there are still some requests
        // in progress so we can't shut down the server right away. We have to wait
        // until the request counter drops down to 0.
        synchronized(request_counter_lock) {
            logger.info("acquired lock for request_counter");
            while(request_counter &gt; 0) {
                logger.info("request counter is above 0 so waiting");
                try {
                    // Wait 1 second for ongoing clients to finish with their work.
                    request_counter_lock.wait(1000);
                } catch (InterruptedException e1) {
                    logger.error("error in request_counter synchronized block: " + e1.getMessage());
                }
            }
        }
        logger.info("request counter reached 0 so going to close server");
        try {
            server.close();
        } catch (IOException e1) {
            logger.error("unable to close server: " + e1.getMessage());
        }
    }
}
I wouldn't recommend creating new Threads on your own or using Threads without holding onto their references. It makes it too hard to shut down properly. In your example, I think that if SocketCloser.close(socket) throws, then the request_counter will be off by one.

I recommend starting with a thread pool.

ExecutorService pool = ExecutorService.newCachedThreadPool();


You can use a fixed size pool if an infinite pool causes you problems. Then you can submit Runnables to it in the same place you're creating Threads now. Finally, when you want to shut down, you can use the methods shutdownNow() and awaitTermination() on the pool.