multiple fields: volatile or AtomicReference?

Question

I have to synchronize access between threads to a shared object, whose state consists of several fields. Say:

class Shared{
String a; Integer b;
//constructor, getters and setters
....
}

I have possibly many threads reading this objects, doing

//readers
shared.getA();
shared.getB();

and only one thread that will write at a certain point:

//writer
shared.setA("state");
shared.setB(1);

now my question is how to ensure that reading threads won't find the shared object in an inconsistent state.

I read many answers saying that for consistency between threads volatile is the solution,but I'm not sure how it works on multiple fields. E.g., is that enough?

volatile  String a; volatile Integer b;

Another solution would be to make the shared object immutable and use AtomicReference, E.g.,

AtomicReference<Shared> shared = ....

and then the writer will just swap the reference:

Shared prev = shared.get(); 
Shared newValue = new Shared("state",1);
while (!shared.compareAndSet(prev, newValue)) 

Is that approach correct? thanks!

Update In my setting the Shared objects are retrieved from a ConcurrentHashMap<Id,Shared>, so the comments agree that the way to go is either using the immutable approach or via synchronizing the updates on shared all together. However, for completeness would be nice to know whether the solution above with the ConcurrentHashMap<Id,AtomicReference<Shared>> is viable or wrong or just superfluous. Anyone can explain? thanks!

Shoemaker

If you have to atomically write to both a and b, then the solution is indeed to use the immutable approach. You don't need an AtomicReference then, just a simple volatile field to your current Shared instance. – Vorfeld

If you make the Shared class immutable and you only have one thread writing then you don't need the atomic compareAndSet, just a volatile Shared shared; would do the job. – Pinkham

What about using ReadWriteLock? – Brock

I see the point of volatile but what if I don't have such reference but I retrieve shared objects from a map<Id,Shared>? How can I use here volatile? Shall I use map<Id,AtomicReference<Shared>> with the immutable approach? – Shoemaker

Answer 1

First of all you should make Shared immutable:

class Shared{
   private final String a; 
   private final int b;
   //constructor, getters and NO setters
}

And if you have only one writer you can safely use volatile, there is no need in AtomicRefference. At the point where information is updated old object should not be modified, but rather a new created and assigned to a volatile refference.

Celibacy

you mean to declare volatile shared and then replace it with a new one? But what if I don't have such reference but I retrieve shared from a map<Id,AtomicReference<Shared>>? – Shoemaker

Yes, that's it. AtomicRefference and CAS are required, when multiple thread are trying to write a reference concurrently. What you are doing is called publication and its possible with just volatile. – Celibacy

ok I see. as for my edit above: what if I don't have such reference but I retrieve shared objects from a map<Id,Shared>? Shall I use here map<Id,AtomicReference<Shared>>? – Shoemaker

Use a ConcurrentHashMap<Integer,Shared> then. – Celibacy

Thanks Mikhail, please bear with me :) I actually use ConcurrentHashMap. Now, say one reader does get on the map and reads, and just after a writer does put of a new object for the same id. The reader as an old copy, doesn't it? – Shoemaker

That's the way immutables work. If you want to modify several fields in the same object(not replacing it), you have to make read/write methods synchronized. Or use ReadWriteLock(as was suggested in comments), which is more effectient in a single writer/multiple readers case. – Celibacy

ok then ReadWriteLock or replace of the object in the map. Just for my information, is the solution with ConcurrentHashMap<Id,AtomicReference<Shared>> wrong at all or just superfluous? – Shoemaker

I think it will be superfluous. – Celibacy

Answer 2

As @Mikhail says in his answer, making Shared immutable and replacing the whole object is a nice approach. If you don't want to or "can't" use that approach for some reason, you can just make sure all the fields on Shared are protected by the same lock, and that they are only ever modified together (see update in my example), then it is impossible for them to be seen in an inconsistent state.

e.g.

class Shared {
  private String a;
  private String b;
  public synchronized String getA() {
    return a;
  }
  public synchronized String getB() {
    return b;
  }
  public synchronized void update(String a, String b) {
    this.a = a;
    this.b = b;
  } 
}

Chacma

Answer 3

If you need to write both A and B together to keep them consistent, e.g. they are a name and a Social Security Number, one approach is to use synchronized everywhere and write a single, combined setter.

public synchronized void setNameAndSSN(String name, int ssn) {
   // do validation checking etc...
   this.name = name;
   this.ssn = ssn;
}

public synchronized String getName() { return this.name; }

public synchronized int getSSN() { return this.ssn; }

Otherwise the reader could "see" the object with the new name but with the old SSN.

The immutable approach makes sense too.

Unsatisfactory

Answer 4

Marking fields volatile or making methods synchronised is not going to ensure atomicity.

Writer should take care of the atomicity.

Writer should call all the setters(that should be updated atomically) inside a synchronized block. synchronized(shared) { shared.setA() shared.setB() ... }

For this to work all the getters in the shared object should be synchronized too.

Flinch

IMO, better to make a single, combined synchronized setter so the writer cannot make a mistake. – Unsatisfactory

Agreed.. But the developer may not always have the prvilege to modify the bean directly. – Flinch