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");
        }
    }

score 1 · Answer 1 · answered Apr 28 '13 at 10:39

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.

score 1 · Accepted Answer · answered Apr 28 '13 at 10:40

1

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.

answered Apr 28 '13 at 10:40

Étienne Miret

6,448
5
24
36

I see. So I should use `synchronized(this)`? – ldam Apr 28 '13 at 10:41
Fixed it. I added a `private final Object lock = new Object();` to my class variables, synchronized on that lock, and used a `lock.wait()` in my `stop()` method and a `lock.notify()` in my exception handler. – ldam Apr 28 '13 at 10:50

score 0 · Answer 3 · edited May 23 '17 at 12:12

0

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

edited May 23 '17 at 12:12

Community

1
1

answered Apr 28 '13 at 10:56

Nate

31,017
13
83
207

1

This won't break out of the `while` loop because the thread is blocking on `SocketChannel sc = server.accept();`. That's why I have to close the server forcefully, to break out from that `accept()` block. – ldam Apr 28 '13 at 10:58
Were it not for that, this is definitely the method I would use. – ldam Apr 28 '13 at 11:00
Gotcha. Can you just use a call to `ServerSocket#close()` in the `stop()` method? Then, your loop will break out with a `SocketException` which you catch, and move on? Is it that you're trying to implement this entirely without exceptions? – Nate Apr 28 '13 at 11:02
I just want to close it down without the user having to worry about any errors, which is why I swallow that exception ONLY when I close down the server. If there are any other errors/exceptions they should be reported properly. If I caught `SocketException` and just ignore it that will mask some legitimate exceptions that could occur, so I just went with the general exception and report all of them. At some stage I will probably deal with more specific exceptions but for now this will do. – ldam Apr 28 '13 at 11:16
I think you can safely catch just the `SocketException`, and have a separate handler for other `IOException` instances (see my update if you're interested). The final code you posted just seems to have too many variables to track, and is a bit too confusing. Any time I see confusing code that's supposed to work in a multi-threaded environment, I get a little nervous. But, that's just me :) – Nate Apr 28 '13 at 11:27
Like I said, `SocketException` can be thrown for other legitimate cases (for example, I believe when a socket is closed a `SocketException` is thrown) but I get what you're saying. At the moment this isn't too high on the priority list, it's just been bugging me for a while (excuse the pun). It may be confusing to you but I understand it :) I'm sure I'll get to clean up at some point, but this is still very early code. Thanks for the help mate :) – ldam Apr 28 '13 at 11:47
Right. Calling `ServerSocket#close()` is what I'm suggesting, which **will** cause the `SocketException` during the `accept()` call. That's what you want. I don't think there's any *other* scenario where `close()` can be called. The client can't call `close()` on the server's socket. If a client simply closes its own client socket, immediately after connecting, you shouldn't get into that catch block (your server code will have already successfully finished the call to `accept()`). Again, the link I included in my answer shows quite a bit of agreement on that being the way to close ... – Nate Apr 28 '13 at 11:52
Oh I see what you're saying now. Also now that I think about it, any other exceptions (including legitimate `SocketException`s) should be handled in that `ReceiveSession`, so you're probably right in saying I should just handle that `SocketException` like that. – ldam Apr 28 '13 at 12:17
My code wasn't playing nicely with android, so I went with the `SocketException` catch and it turns out `java.nio.AsynchronousCloseException` is not a `SocketException`. I guess I'll just catch that one then. – ldam Apr 28 '13 at 13:48

How do I fix this race condition?

3 Answers3

Linked