Are Mutable Atomic References a Bad Idea?

Question

I have a data structure that I occasionally wish to modify, and occasionally wish to replace outright. At the moment, I'm storing this in an AtomicReference, and using synchrnonized blocks (synchronized on the AtomicReference itself, not its stored value) when I need to modify it, rather than replace it.

So something like:

public void foo(AtomicReference reference){
    synchronized(reference){
        reference.get()
            .performSomeModification();
    }
}

Notice that the modifying call is a member of the wrapped value, not the atomic reference, and is not guaranteed to have any thread safety of its own.

Is this safe? Findbugs (a freeware code reviewing tool) had this to say about it, so now I'm worried there's something happening under the hood, where it may be prematurely releasing the lock or something. I've also seen documentation referencing AtomicReference as specifically for immutable things.

Is this safe? If it isn't I could create my own Reference-storing class that I would be more certain about the behavior of, but I don't want to jump to conclusions.

Answer 1

From the linked documentation:

For example, synchronizing on an AtomicBoolean will not prevent other threads from modifying the AtomicBoolean.

It can't prevent other threads from modifying the AtomicBoolean because it can't force other threads to synchronize on the AtomicBoolean.

If I understand your question correctly, your intention is to synchronize calls to performSomeModification(). The code you've written will achieve that, if and only if every call to performSomeModification() is synchronized on the same object. As in the example from the docs, the basic problem is the enforceability of that requirement. You can't force other callers to synchronize on the AtomicReference. You or some other developer who comes after you could easily call performSomeModification() without external synchronization.

You should make it hard to use your API incorrectly. Since AtomicReference is a generic type (AtomicReference<V>), you can enforce the synchronization in a variety of ways, depending on what V is:

• If V is an interface, you could easily wrap the instance in a synchronized wrapper.
• If V is a class that you can modify, you could synchronize performSomeModification(), or create a subclass in which it is synchronized. (Possibly an anonymous subclass produced by a factory method.)
• If V is a class that you cannot modify, it may be difficult to wrap. In that case, you could encapsulate the AtomicReference in a class that you do control, and have that class perform the required synchronization.

Answer 2

Are Mutable Atomic References a Bad Idea?

Definitely not! AtomicReference is designed to provide thread-safe, atomic updates of the underlying reference. In fact, the Javadoc description of AtomicReference is:

An object reference that may be updated atomically.

So they most definitely are designed to be mutated!

Is this safe?

It depends on what you mean by "safe", and what the rest of your code is doing. There's nothing inherently unsafe about your snippet of code in isolation. It's perfectly valid, though perhaps a bit unusual, to synchronize on an AtomicReference. As a developer unfamiliar with this code, I would see the synchronization on reference and assume that it means that the underlying object may be replaced at any time, and you want to make sure your code is always operating on the "newest" reference.

The standard best practices for synchronization apply, and violating them could result in unsafe behavior. For example, since you say performSomeModification() is not thread-safe, it would be unsafe if you accessed the underlying object somewhere else without synchronizing on reference.

public void bar(AtomicReference reference) {
    // no synchronization: performSomeModification could be called on the object 
    // at the same time another thread is executing foo()
    reference.get().performSomeModification();

}

If could also be "unsafe" if your application requires that only one instance of the underlying object be operated on at any one time, and you haven't synchronized on the reference when .set()ing it:

public void makeNewFoo(AtomicReference reference) {
    // no synchronication on "reference", so it may be updated by another thread 
    // while foo() is executing performSomeModification() on the "old" reference
    SomeObject foo = new SomeObject();
    reference.set(foo);
}

If you need to synchronize on the AtomicReference, do so, it's perfectly safe. But I would highly recommend adding a few code comments about why you're doing it.