1
\$\begingroup\$

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 > 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());
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Could you please clarify who is responsible for changing exit flag ? Currently looks like you have an infinite loop. \$\endgroup\$ – stoweesh Nov 14 '12 at 22:27
  • \$\begingroup\$ Is "try-catch with resources" available? \$\endgroup\$ – oopexpert Feb 6 '17 at 12:34
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ You make a good point about exceptions in the finally block but I make sure that SocketCloser.close doesn't throw any exceptions. Everything is logged and handled in SocketCloser.close so the finally block won't throw any exceptions related to closing a socket. \$\endgroup\$ – davidk01 Jul 21 '11 at 14:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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