javamultithreadingsynchronizedatomicreference

Are Mutable Atomic References a Bad Idea?

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.

Solution

  • 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: