Java multithreaded server *sometimes* throws SocketException (Socket closed) in ServerSocket.accept() method

I've been investigating this issue for hours and hours and I cannot come up with a decent solution or an explanation as to why this exception (java.net.SocketException: Socket closed) is getting thrown. My last approach is now to ask you guys.

I've created a simple server-client app for testing purposes (the 'real' application uses the same logic though), see below.

If I repeatably invoke the same test-case (for example through TestNG's invocationcount annotation parameter or by using a simple for-loop), at some point there will be a java.net.SocketException: Socket closed.

The test case below basically just starts the server (opens the server socket), waits a couple of millis and then closes socket again. Closing the server socket involves opening a socket so that the server will return from the ServerSocket.accept() method (see Server#shutdown()).

I though it might be a multithreading issue with the code right after the ServerSocket.accept()-line. So I've surrounded it temporarily with a synchronized-block - didn't help either.

Do you have any idea why this exception gets thrown?

Best, Chris

Server.java looks like this:

package multithreading;
import java.io.IOException;
import java.net.ServerSocket;
import java.net.Socket;
import org.apache.log4j.Logger;

public class Server {

private ServerSocket serverSocket;
private boolean isShuttingDown;
private final static Logger logger = Logger.getLogger(Server.class);

public void start() throws Exception {
    try {
        serverSocket = new ServerSocket(5000);
        isShuttingDown = false;
    } catch (Exception e) {
        throw new RuntimeException("Starting up the server failed - aborting", e);
    }

    while (true) {
        try {
            Socket socket = serverSocket.accept();

            if (!isShuttingDown) {
                new Thread(new EchoRequestHandler(socket)).start();
            } else {
                logger.info("Server is going to shutdown");
                break;
            }
        } catch (IOException e) {
            logger.error("Error occured while waiting for new connections, stopping server", e);
            throw e;
        }
    }
}

public synchronized boolean isRunning() {
    if (serverSocket != null && serverSocket.isBound() && !serverSocket.isClosed() && !isShuttingDown) {
        return true;
    }
    return false;
}

public synchronized void shutdown() throws IOException {
    if (isRunning()) {
        isShuttingDown = true;
        if (serverSocket != null && !serverSocket.isClosed()) {
            try {
                /*
                 * since the server socket is still waiting in it's accept()
                 * method, just closing the server socket would cause an
                 * exception to be thrown. By quickly opening a socket
   开发者_如何学运维              * (connection) to the server socket and immediately closing
                 * it again, the server socket's accept method will return
                 * and since the isShuttingDown flag is then false, the
                 * socket will be closed.
                 */
                new Socket(serverSocket.getInetAddress(), serverSocket.getLocalPort()).close();

                serverSocket.close();
            } catch (IOException e) {
                logger.error("Closing the server socket has failed - aborting now.", e);
                throw e;
            }
        }
    } else {
        throw new IOException("Server socket is already closed which should not be the case.");
    }
}
}

The test class does the following:

package multithreading;

import java.io.IOException;

import org.testng.annotations.Test;

public class Testing {

// @Test(invocationCount=10, skipFailedInvocations=true)
@Test
public void loadTest() throws InterruptedException, IOException {
    for (int i = 0; i < 10; i++) {
        final Server s = new Server();
        new Thread(new Runnable() {
            @Override
            public void run() {
                try {
                    s.start();
                } catch (Exception e) {
                    e.printStackTrace();
                }
            }

        }).start();
        Thread.sleep(500);
        gracefullyShutdownServer(s);
        Thread.sleep(1000);
    }
}

private void gracefullyShutdownServer(final Server server) throws InterruptedException {
    try {
        server.shutdown();
        while (server.isRunning()) {
            Thread.sleep(500);
        }
    } catch (IOException e) {
        System.err.println(e);
    }
}

}

The stack trace looks as follows:

ERROR 2011-03-13 16:14:23,537 [Thread-6] multithreading.Server: Error occured while waiting for new connections, stopping server
java.net.SocketException: Socket closed
at java.net.PlainSocketImpl.socketAccept(Native Method)
at java.net.PlainSocketImpl.accept(PlainSocketImpl.java:390)
at java.net.ServerSocket.implAccept(ServerSocket.java:453)
at java.net.ServerSocket.accept(ServerSocket.java:421)
at multithreading.Server.start(Server.java:26)
at multithreading.Testing$1.run(Testing.java:18)
at java.lang.Thread.run(Thread.java:680)
java.net.SocketException: Socket closed
at java.net.PlainSocketImpl.socketAccept(Native Method)
at java.net.PlainSocketImpl.accept(PlainSocketImpl.java:390)
at java.net.ServerSocket.implAccept(ServerSocket.java:453)
at java.net.ServerSocket.accept(ServerSocket.java:421)
at multithreading.Server.start(Server.java:26)
at multithreading.Testing$1.run(Testing.java:18)
at java.lang.Thread.run(Thread.java:680)

J.N. is correct concerning the way you should be handling the close.

As for the why this is not working, I think the race is that your server code reads isShuttingDown with no synchronization. I don't see why the value change should be visible immediately to the server thread. So it might very well go for another round.

So as J.N. said: deal with the exception in the server when accepting. If you want to know whether the exception is probably being raised by your code doing close on the socket, keep your isShuttingDown around be make sure you access it safely. (Either a synchronized (this) {} block, or write a very short synchronized accessor.)

In this specific case I think making isShuttingDown volatile is sufficient, as detailed in this developerWorks article Java theory and practice: Managing volatility. But be careful with that, it's not a magic bullet.

As specified in this javadoc link, Opening a socket to close another one is a mistake. Calling close on your socket should cancel the "accept" on its own by throwing an exception you can catch and ignore safely.