5

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();
  }
salmonalive
  • 73
  • 1
  • 4
  • No, not required. Any form of safe publication will work (but a raw reference is *not* OK). C. f. this Stack Overflow question: http://stackoverflow.com/questions/801993/java-multi-threading-safe-publication – markspace Dec 01 '15 at 05:06
  • @markspace Would you recommend a volatile variable instead? – salmonalive Dec 01 '15 at 05:12
  • Actually, this one can be tricky. `countDow()` is guaranteed to *happen-before* any call to `await()`. It is possible to use a raw reference here, because every every write of `_result` is followed by a call to `countDown`, and the read of `_result` is preceded by a call to `await`. So `_result` actually can use the `_latch`s visibility semantics. Brian Goetz calls this piggy backing. http://stackoverflow.com/questions/18732088/how-does-the-piggybacking-of-current-thread-variable-in-reentrantlock-sync-work – markspace Dec 01 '15 at 05:14
  • 1
    Actually, if this is existing, working code, I'd recommend not changing it. You won't gain anything. If you're asking hypothetically, then volatile will work. Piggy-backing works too, but it's hard to spot. A maintenance programmer could easily miss it. I would recommend having the code reviewed formally. If no one understands piggy-backing, then leave it as AtomicReference. – markspace Dec 01 '15 at 05:17
  • @Markspace you're rather confusing there. One of the main points of the synchronization latch is to offer visibility guarantees. If you started to not count on those, you'd have to use AtomicReferences or volatile everywhere. Clearly a raw reference is the right choice here and leaving something else will just confuse future readers. – Voo Dec 01 '15 at 09:18
  • @Voo the visibility is only guaranteed when count is not 0 - so technically the class would not be thread safe without the result being `volatile`. – assylias Dec 01 '15 at 10:15
  • @assylias You mean if more than one setter is called? True, I'm assuming a single initialisation - everything else would be weird. – Voo Dec 01 '15 at 13:18
  • @Voo Yes agreed, it's probably not intended but is possible. – assylias Dec 01 '15 at 13:22
  • @Voo I said that I think a maintainer might miss it--a lot of folks don't know about piggy-backing. In my opinion anyway. It's a personnel issue, not a technical one. – markspace Dec 01 '15 at 17:18
  • @markspace I doubt that you can write concurrent code that can be maintained by someone who doesn't know one of the most basic facts of visibility. That seems like a lost cause. On the other hand you can certainly make it easier for someone who knows what he's doing. If I saw that class with the AtomicReference I would assume that it existed because the onXXX methods might be called repeatedly - which would rather confuse me (set both result and exception? What weird corner case is this?). – Voo Dec 01 '15 at 17:22
  • @markspace As soon as the count reaches zero, additional countDowns don't give you the wanted visibility guarantees. Also "piggybacking" is just an informal description that Brian uses to describe the behavior - it's not really a term most people would know. – Voo Dec 01 '15 at 17:23
  • @Voo Oops, you're right. I had to go back and read the docs. Yeah that's a hole in this class's design. (See? I said piggy-backing was hard to reason about. AtomicReference is simpler!) – markspace Dec 01 '15 at 17:25
  • @markspace Strikes me as perfectly reasonable for what the class is designed to do and follows from how the class is assumed to be implemented (there really are only so many ways you can implement a countdownlatch after all). Yes concurrency is complicated, but just throwing things at it for good measure is not a solution, you just make the reasoning harder without any improvements. – Voo Dec 01 '15 at 17:29

4 Answers4

2

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.

Zbynek Vyskovsky - kvr000
  • 18,186
  • 3
  • 35
  • 43
  • It depends if the setters are called more than once or not - if they are then there is no visibility guarantee and you need extra synchronization. – assylias Dec 01 '15 at 11:01
1

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.

assylias
  • 321,522
  • 82
  • 660
  • 783
0

In order for an assignment to be visible across threads some sort of memory barrier must be crossed. This can be accomplished several different ways, depending on what exactly you're trying to do.

  • You can use a volatile field. Reads and writes to volatile fields are atomic and visible across threads.
  • You can use an AtomicReference. This is effectively the same as a volatile field, but it's a little more flexible (you can reassign and pass around references to the AtomicReference) and has a few extra operations, like compareAndSet().
  • You can use a CountDownLatch or similar synchronizer class, but you need to pay close attention to the memory invariants they offer. CountDownLatch, for instance, guarantees that all threads that await() will see everything that occurs in a thread that calls countDown() up to when countDown() is called.
  • You can use synchronized blocks. These are even more flexible, but require more care - both the write and the read must be synchronized, otherwise the write may not be seen.
  • You can use a thread-safe collection, such as a ConcurrentHashMap. Overkill if all you need is a cross-thread reference, but useful for storing structured data that multiple threads need to access.

This isn't intended to be a complete list of options, but hopefully you can see there are several ways to ensure a value becomes visible to other threads, and that AtomicReference is simply one of those mechanisms.

Community
  • 1
  • 1
dimo414
  • 47,227
  • 18
  • 148
  • 244
  • Or you can also use piggy-backing. ;-) Which a "raw reference" would do in this case. C.f. *Java Concurrency in Practice* by Brian Goetz. – markspace Dec 01 '15 at 05:20
  • `CountDownLatch` is not a "thread control class." It uses the same *synchronizes-with* and *happens-before* semantics that every other one of your examples use. In other words, piggy-backing happens naturally in all of those cases and could be used by any of them. – markspace Dec 01 '15 at 05:25
  • @markspace thank you for the clarifications. While all of these are examples of the same memory semantics, they are different in practice (i.e. you would likely prefer one or another, depending on the specific use case). My intent here is simply to call out some alternative tools. – dimo414 Dec 01 '15 at 05:31
  • No worries and I hope I didn't sound snarky or whatever. I just want to be careful with terminology. People read words and get funny ideas, and they tend to carry around those funny ideas without ever correcting them. It's important to understand that in the OPs case piggy-backing works, and why it works is because happens-before is transitive. It always works like that (except for final fields, which aren't transitive). – markspace Dec 01 '15 at 05:48
0

Short answer is you don't need AtomicReference here. You'll need volatile though.

The reason is that you're only writing to and reading from the reference (Result) and not doing any composite operations like compareAndSet().

Reads and writes are atomic for reference variables and for most primitive variables (all types except long and double).

Reference, Sun Java tutorial
https://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html

Then there is JLS (Java Language Specification)

Writes to and reads of references are always atomic, regardless of whether they are implemented as 32-bit or 64-bit values.

Java 8
http://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.7
Java 7
http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.7
Java 6
http://docs.oracle.com/javase/specs/jls/se6/html/memory.html#17.7

Source : https://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html

Atomic actions cannot be interleaved, so they can be used without fear of thread interference. However, this does not eliminate all need to synchronize atomic actions, because memory consistency errors are still possible. Using volatile variables reduces the risk of memory consistency errors, because any write to a volatile variable establishes a happens-before relationship with subsequent reads of that same variable. This means that changes to a volatile variable are always visible to other threads. What's more, it also means that when a thread reads a volatile variable, it sees not just the latest change to the volatile, but also the side effects of the code that led up the change.

Since you have only single operation write/read and it's atomic, making the variable volatile will suffice.

Regarding use of CountDownLatch, it's used to wait for n operations in other threads to complete. Since you have only one operation, you can use Condition, instead of CountDownLatch.

If you're interested in usage of AtomicReference, you can check Java Concurrency in Practice (Page 326), find the book below:

https://github.com/HackathonHackers/programming-ebooks/tree/master/Java

Or the same example used by @Binita Bharti in following StackOverflow answer
When to use AtomicReference in Java?

Community
  • 1
  • 1
11thdimension
  • 10,333
  • 4
  • 33
  • 71