Is AtomicReference needed for visibility between threads?

Question

I'm working with a framework that requires a callback when sending a request. Each callback has to implement this interface. The methods in the callback are invoked asynchronously.

public interface ClientCallback<RESP extends Response>
{
  public void onSuccessResponse(RESP resp);

  public void onFailureResponse(FailureResponse failure);

  public void onError(Throwable e);
}

To write integration tests with TestNG, I wanted to have a blocking callback. So I used a CountDownLatch to synchronize between threads.

Is the AtomicReference really needed here or is a raw reference okay? I know that if I use a raw reference and a raw integer (instead of CountDownLatch), the code wouldn't work because visibility is not guaranteed. But since the CountDownLatch is already synchronized, I wasn't sure whether I needed the extra synchronization from AtomicReference. Note: The Result class is immutable.

public class BlockingCallback<RESP extends Response> implements ClientCallback<RESP>
{
  private final AtomicReference<Result<RESP>> _result = new AtomicReference<Result<RESP>>();
  private final CountDownLatch _latch = new CountDownLatch(1);

  public void onSuccessResponse(RESP resp)
  {
    _result.set(new Result<RESP>(resp, null, null));
    _latch.countDown();
  }

  public void onFailureResponse(FailureResponse failure)
  {
    _result.set(new Result<RESP>(null, failure, null));
    _latch.countDown();
  }

  public void onError(Throwable e)
  {
    _result.set(new Result<RESP>(null, null, e));
    _latch.countDown();
  }

  public Result<RESP> getResult(final long timeout, final TimeUnit unit) throws InterruptedException, TimeoutException
  {
    if (!_latch.await(timeout, unit))
    {
      throw new TimeoutException();
    }
    return _result.get();
  }

Answer 1

You don't need to use another synchronization object (AtomicRefetence) here. The point is that the variable is set before CountDownLatch is invoked in one thread and read after CountDownLatch is invoked in another thread. CountDownLatch already performs thread synchronization and invokes memory barrier so the order of writing before and reading after is guaranteed. Because of this you don't even need to use volatile for that field.

Answer 2

A good starting point is the javadoc (emphasis mine):

Memory consistency effects: Until the count reaches zero, actions in a thread prior to calling countDown() happen-before actions following a successful return from a corresponding await() in another thread.

Now there are two options:

1. either you never call the onXxx setter methods once the count is 0 (i.e. you only call one of the methods once) and you don't need any extra synchronization
2. or you may call the setter methods more than once and you do need extra synchronization

If you are in scenario 2, you need to make the variable at least volatile (no need for an AtomicReference in your example).

If you are in scenario 1, you need to decide how defensive you want to be:

• to err on the safe side you can still use volatile
• if you are happy that the calling code won't mess up with the class, you can use a normal variable but I would at least make it clear in the javadoc of the methods that only the first call to the onXxx methods is guaranteed to be visible

---

Finally, in scenario 1, you may want to enforce the fact that the setters can only be called once, in which case you would probably use an AtomicReference and its compareAndSet method to make sure that the reference was null beforehand and throw an exception otherwise.