Solving a race condition

In my application, I have a message dispatcher. Each message gets relayed to a dispatcher thread.

In some scenarios, I can get two responses from the third party in the same millisecond:

• An "I received your message" ACKnowledgement

• "My response to your message" RESPONSE

When I get both messages at the same time, this can create a race condition. It is important to have my EventHandler handle the ACK before I get the RESPONSE, but I cannot guarantee ordering from the Asynchronous networking IO library; I must do it myself.

Below is a working MVCE of this scenario. Most of it is just framework, it's not how it works in the real code, but it's enough to get the bit I'm asking about - the ordering logic - to show it's value. The Proposed Solution to this race condition is the ensureAck method, which is called in the handleResponse method. If you comment out that call, and run the application repeatedly, you will see that there is no guarantee of method ordering. Additionally, and this is the difficult bit: I don't want the RESPONSE to be ignored if, for some reason, the ACK never shows up (a dropped network packet or something) - I want the RESPONSE to get handled, reluctantly, eventually.

Aside: the bit about NOTHING messages are just to warm up the ExecutorService so you can actually see the race condition.

How is my solution?

```
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class RaceSolution {
private static enum State {
NEW, READY;
}

private static enum Type {
ACK, RESPONSE, NOTHING;
}

private static class EventHandler {
private volatile State state = State.NEW;
private final Object lock = new Object();

public void handleEvent(Event event) {
switch(event.t) {
case ACK:
handleAck(event);
break;
case RESPONSE:
handleResponse(event);
break;
default:
}
}

private void handleResponse(E

Concurrency Strategy

In general, using volatile is complicated, partially because the meaning of volatile changed in Java 1.4, and also because it is hard to spot. I recommend against using it at all. Instead, you should use a more visible concurrency mechanism like synchronization, java.util.concurrent. classes, java.util.concurrent.locks., or java.util.concurrent.atomics.*

So, while I recommend against volatile, what's a real problem is using multiple different locking schemes in the same code, and you use both volatile and synchronized.

The use of synchronization as your concurrency strategy should be fine all on its own in this case.

General

-
The private final Object lock = new Object() is great. Good to be final, and I prefer that to locking on the instance itself (like synchronized(this)). Makes it more clear.

-
Println in a synchronized block is almost never a good idea:

synchronized(lock) {
  System.out.println("Received ack Event: " + state);
  state = State.READY;
}

A better solution would be:

State copy = null;
synchronized(lock) {
  copy = state;
  state = State.READY;
}
System.out.println("Received ack Event: " + copy);

-
Timed sleeps while waiting for a lock are a horrible solution:

Thread.sleep(1);

Better algorithm

The right way to solve this problem, though is to used timed-waits on the lock:

private void handleAck(Event event) {
    State copy = null;
    synchronized(lock) {
      copy = state;
      state = State.READY;
      // tell any waiting threads the state has changed.
      lock.notifyAll();
    }
    System.out.println("Received ack Event: " + copy);
}

/* THIS IS THE IMPORTANT BIT */
private void ensureAck() {
  // how long to wait... 10 milliseconds
  try {
      synchronized(lock) {
          if (state == State.NEW) {
              long waited = 0; // milliseconds
              long start = System.currentTimeMillis();
              while (state == State.NEW && waited < 10) {
                  lock.wait(10 - waited);
                  waited = System.currentTimeMillis() - start;
              }
          }
      }
  } catch (InterruptedException e) {
      // do more than nothing.....
  }
}

The above strategy does not need anything to be volatile, and it does not 'poll' the state, it returns immediately when the state changes....

StackExchange Code Review Q#66907, answer score: 7