CODEWITHSUNDEEP
javamultithreadingrace-condition
Chris Smith
Chris Smith

Reputation: 3012

Weird race condition?

I am having an issue with the method I wrote. What is does, is wait for a property to equal another value. I can't reproduce it here, but in the unit test I am running, I want to wait for a particular value to be true. It prints out "wait", and then "notify" (indicating that it should be working correctly), but "done waiting" is never printed. It doesn't always do it, and sometimes it works flawlessly, this is why I think it's some sort of race condition.

I was using the Locking API before, but was getting the same issues so I decided to try the old wait/notify stuff.

Could it possibly be the listener being created and called, before it has a chance to wait? Even if this was the issue, it shouldn't print "wait", and then "notify". Even if I print the ms at the time of "wait" and "notify", wait still comes before notify.

private static <T> void waitFor(ObservableValue<T> value, T wantedValue) throws InterruptedException {
    if (value.getValue() != wantedValue) {
        Object lock = new Object();
        synchronized (lock) {
            value.addListener((observableValue, t, t1) -> {
                if (value.getValue() != wantedValue) {
                    return;
                }
                synchronized (lock) {
                    System.out.print("notify");
                    lock.notifyAll();
                }
            });
            System.out.println("wait");
            lock.wait();
            System.out.println("done waiting");
        }
    }
}

Upvotes: 0

Views: 188

Answers (2)

JimmyB
JimmyB

Reputation: 12630

    Object lock = new Object();
    synchronized (lock) { ...

does not make sense.

Multiple threads must use the same object to synchronize on, or the synchronization is just not happening.

At the end, your code keeps waiting for a signal which no other thread could ever send to the lock object, because that object is local to the method.

Edit: Please ignore the above. It's just based on my overlooking of that the lock object is passed to the anonymous listener which does get handed out of the method.

However, another potential race condition is in

if (value.getValue() != wantedValue) {
    Object lock = new Object();
    synchronized (lock) { ...

What if the value changes asynchronously after getValue() but before value.addListener(...) is executed?

And another minor issue: The documentation for wait() states "spurious wakeups are possible, and this method should always be used in a loop" (emphasis mine).

Upvotes: 4

Jonathan Locke
Jonathan Locke

Reputation: 125

The comments about your lock so far are overlooking an important detail. Your lock variable is actually NOT a local variable. It is "effectively final" in Java 8 because it's referenced inside your listener closure. This means it's actually being copied into a special compiler-generated field in the anonymous class implementing the closure object and because of this, it is actually potentially shared between the thread that calls waitFor() and whatever thread might call the registered listener closure. So, I think this is not the problem at all.

However, depending on what you're expecting to happen, there is a possible race condition in your code. The listener is being added before you call wait(), which means that if there is an immediate asynchronous notification before you can even get to wait(), the listener's call to notifyAll() will be missed and the code will wait forever for the notification that already happened. Of course, if you're expecting more notifications in the future, this isn't really a problem because you'll unblock when the next notification comes along, but I expect you're seeing this bug in a unit test where you're waiting for only one notification and you're simply missing it sometimes which is causing your test to fail sometimes.

One further thought. Maybe you could change your code to:

ObservableValue.waitFor(T value)

Code is almost always better without the static keyword (I wish it didn't exist). And if ObservableValue is an interface, you can always make waitFor() a default method since you're in Java 8.

Upvotes: 2

Related Questions