5

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!

legrass
  • 407
  • 5
  • 13
  • 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. – Voo Jan 22 '14 at 12:44
  • 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. – Ian Roberts Jan 22 '14 at 12:46
  • What about using [ReadWriteLock](http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReadWriteLock.html)? – Fildor Jan 22 '14 at 12:56
  • I see the point of volatile but what if I don't have such reference but I retrieve shared objects from a map? How can I use here volatile? Shall I use map> with the immutable approach? – legrass Jan 22 '14 at 13:04

4 Answers4

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.

Mikhail
  • 4,175
  • 15
  • 31
  • 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>? – legrass Jan 22 '14 at 12:55
  • 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. – Mikhail Jan 22 '14 at 12:57
  • ok I see. as for my edit above: what if I don't have such reference but I retrieve shared objects from a map? Shall I use here map>? – legrass Jan 22 '14 at 12:59
  • Use a ConcurrentHashMap then. – Mikhail Jan 22 '14 at 13:02
  • 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? – legrass Jan 22 '14 at 13:17
  • 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. – Mikhail Jan 22 '14 at 13:36
  • ok then ReadWriteLock or replace of the object in the map. Just for my information, is the solution with ConcurrentHashMap> wrong at all or just superfluous? – legrass Jan 22 '14 at 15:35
  • I think it will be superfluous. – Mikhail Jan 23 '14 at 05:57
1

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;
  } 
}
overthink
  • 23,985
  • 4
  • 69
  • 69
1

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.

user949300
  • 15,364
  • 7
  • 35
  • 66
1

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.

Praneeth Reddy G
  • 448
  • 2
  • 7