How do I fix this race condition?

Question

I have a server accepting clients that has a stop() method that closes the server down, which is causing a java.nio.AsynchronousCloseException that I'd like to resolve. The stop() method is called on a different thread, which is what is causing the race condition I believe.

Here is my code:

public void run() {
    InetSocketAddress addr = new InetSocketAddress(provider.getConnection(), 12354);
    try {
        server = ServerSocketChannel.open();
        server.configureBlocking(true);
        server.socket().bind(addr);
        parent.setIP(addr.getAddress().getHostAddress().toString());
        password = generatePassword();
        parent.setPassword(password);
        parent.setStatus("Ready.");
    } catch (IOException e) {
        parent.die("Could not start server: " + e.getMessage());
        runner = null;
    }
    while (runner == Thread.currentThread()) {
        try {
            SocketChannel sc = server.accept();
            if (available) {
                session = new ReceiveSession(this, sc, password, addr.getAddress());
                session.start();
                available = false;
            } else {
                new ReceiveBusyHandler(sc).start();
            }
        } catch (IOException e) {
            synchronized (swallowException) {
                if (!swallowException) {
                    parent.showError(e.toString());
                }
                available = true;
            }
        }
    }
}

public void stop() throws IOException {
    synchronized (swallowException) {
        swallowException = true;
        runner = null;
        if (server != null) {
            server.socket().close();
            server.close();
        }

        swallowException = false;
        System.out.println("Server down");
    }
}

(FYI, swallowException is a Boolean and you can see I've tried synchronizing it.)

It looks like the stop() method is setting swallowException to true and then back to false before the exception handler in my server loop has a chance to access it.

UPDATE: I introduced a new Object to use as a lock, and used wait()/notify() to fix my issue:

public void run() {
        InetSocketAddress addr = new InetSocketAddress(provider.getConnection(), 12354);
        try {
            server = ServerSocketChannel.open();
            server.configureBlocking(true);
            server.socket().bind(addr);
            parent.setIP(addr.getAddress().getHostAddress().toString());
            password = generatePassword();
            parent.setPassword(password);
            parent.setStatus("Ready.");
        } catch (IOException e) {
            parent.die("Could not start server: " + e.getMessage());
            runner = null;
        }
        while (runner == Thread.currentThread()) {
            try {
                SocketChannel sc = server.accept();
                if (available) {
                    session = new ReceiveSession(this, sc, password, addr.getAddress());
                    session.start();
                    available = false;
                } else {
                    new ReceiveBusyHandler(sc).start();
                }
            } catch (IOException e) {
                synchronized (lock) {
                    if (!swallowException) {
                        parent.showError(e.toString());

                    }
                    lock.notify();
                    available = true;
                }
            }
        }
    }

    public void stop() throws IOException {
        synchronized (lock) {
            swallowException = true;
            runner = null;
            if (server != null) {
                server.socket().close();
                server.close();
            }
            while (swallowException) {
                try {
                    lock.wait();
                    swallowException = false;
                } catch (InterruptedException e) {
                }
            }
            //swallowException = false;
            System.out.println("Server down");
        }
    }

Answer 1

I would strongly advise refactoring your code (even the solution you posted as an update), because it's just not clear what's happening.

From your description, it seems like you just want a thread-safe way to stop your server. I recommend doing this, whereby you simply call close() on the ServerSocket, and be able to catch a SocketException:

private boolean cont = true;

// this can safely be called from any thread
public synchronized void stop() {
    cont = false;
    if (server != null) {
       server.socket().close();
    }
}
private synchronized void setContinue(boolean value) {
    cont = value;
}
private synchronized boolean shouldContinue() {
    return cont;
}
private synchronized void openChannel() {
    server = ServerSocketChannel.open();
}

public void run() {
    InetSocketAddress addr = new InetSocketAddress(provider.getConnection(), 12354);
    try {
        openChannel();
        server.configureBlocking(true);
        server.socket().bind(addr);
        parent.setIP(addr.getAddress().getHostAddress().toString());
        password = generatePassword();
        parent.setPassword(password);
        parent.setStatus("Ready.");
    } catch (IOException e) {
        parent.die("Could not start server: " + e.getMessage());
        setContinue(false);
    }

    while (shouldContinue()) {
        try {
            SocketChannel sc = server.accept();
            if (shouldContinue()) {
                if (available) {
                    session = new ReceiveSession(this, sc, password, addr.getAddress());
                    session.start();
                    available = false;
                } else {
                    new ReceiveBusyHandler(sc).start();
                }
            }
        } catch (SocketException se) {
            // normal shutdown from stop()
        } catch (IOException e) {
            parent.showError(e.toString()); 
            available = true;               
        }
    }
    System.out.println("Server down");
}

See more about this technique for stopping a server here

Answer 2

In Java, synchronization is done on an object, not on a variable. When you synchronize on swallowException, you synchronize on its value (Boolean.TRUE or Boolean.FALSE). This is not what you want. You should synchronize on the object that contains swallowException.

Answer 3

This part is not correctly synchronized:

synchronized (swallowException) {
    swallowException = true;

You are synchronizing on one instance (false) and immediately changing the swallowException reference to point to a different instance (true). The next thread to enter stop won't block.

Either synchronize on an instance that won't be swapped out (the owner of these methods) or use some other locking mechanism from java.util.concurrent.