1

I have some classes that contains both immutable (primitive & references) and mutable data, but mutability come from external writes (another process).

To avoid using synchronized on every getters/setters (and around the update operation, atomicity is important) or a ReadWriteLock, I came up with this solution that rely on a volatile field pointing to a inner class instance:

class A {

    private class AData {

        final int foo;

        AData(X x, ...) {
            foo = ...
        } 

   }

   private final int a;
   private final B b;

   private volatile AData data;

   A(int a, B b, X x, Y y, ...) {
       this.a = a;
       this.b = b;
       data = new AData(x, y, ...);
       ...
   }

   public int getFoo() {
       return data.foo;
   }

   void update(X x, Y y...) {
       data = new AData(x, y, ...);
   }

}

It looks to me as a good compromise for my use case (a lot more reads, from different threads, than updates), but I'm just trying to make sure I didn't fall into a known anti-pattern.

  • 2
    Your AData.foo is not final. I hope it is a typo, otherwise the solution is probably not thread-safe yet. – yegodm Mar 22 '18 at 18:37
  • 5
    Style wise, I think using AtomicReference<AData> rather than volatile might be better. It probably makes little difference to actual behaviour (although it will give you a number of extra options for how to make updates, e.g. atomic test and set so that an operation can fail if another thread has made a change while preparing an update), but it makes your intent that data is changed atomically easier to see, which is particularly important in code like this that can go wrong very easily if a developer misunderstands what it is doing. – Jules Mar 22 '18 at 20:21
  • Depending on how it is used, the user of your data type will probably have to synchronize all access anyway, to avoid race conditions between getFoo and update. So you may be making something thread-safe at the wrong level. I am not sure about Java, but volatile is considered deprecated in C++ and bad form in C#. – Frank Hileman Mar 22 '18 at 21:37
  • @FrankHileman, Re, "Depending on how it is used..." The reason to use synchronized instead of AtomicReference would be if there is some other variable that has to always be consistent with the data variable. You would use synchronized in that case to ensure that other threads always see both variables change in one atomic step. But if there's only one variable, and you only want to ensure that other threads always see the most recent version of it,... that's exactly the case that AtomicReference (and AtomicEverythingelse) was meant for. – Solomon Slow Apr 25 '18 at 13:48
  • @jameslarge Correct, I was just pointing out a common error I often find in multithreaded code (synchronizing at the wrong level). – Frank Hileman Apr 25 '18 at 17:14
1

As far as I understand, you first build a replacement AData object, then atomically update the read-accessible reference so that it points to the newly-created AData object.

The idea looks fine to me, as long as there's no more than writer.

With several writers contending to .update, I'd make that method synchronized. This would only affect writers, never readers.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.