6

For educational purposes I'm writing a simple version of AtomicLong, where an internal variable is guarded by ReentrantReadWriteLock. Here is a simplified example:

public class PlainSimpleAtomicLong {

  private long value;

  private final ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock();

  public PlainSimpleAtomicLong(long initialValue) {
    this.value = initialValue;
  }

  public long get() {
    long result;

    rwLock.readLock().lock();
    result = value;
    rwLock.readLock().unlock();

    return result;
  }

  // incrementAndGet, decrementAndGet, etc. are guarded by rwLock.writeLock()
}

My question: since "value" is non-volatile, is it possible for other threads to observe incorrect initial value via PlainSimpleAtomicLong.get()? E.g. thread T1 creates L = new PlainSimpleAtomicLong(42) and shares reference with a thread T2. Is T2 guaranteed to observe L.get() as 42?

If not, would wrapping this.value = initialValue; into a write lock/unlock make a difference?

toniedzwiedz
  • 17,895
  • 9
  • 86
  • 131
Pavel Grushetzky
  • 181
  • 1
  • 7
  • 2
    It depends on how you publish the object. If it is not published safely then a thread may observe 42 or 0 when calling get. – assylias Jun 01 '14 at 21:20
  • Note that because you have a final variable and because of the way jvms are currently implemented, it is unlikely that you would be able to create an example that prints 0 - but that doesn't mean it couldn't happen with future versions. – assylias Jun 01 '14 at 21:26
  • @assylias I have a similar thoughts after reading JLS #17.4. Curious though whether lock in the constructor would guarantee visibility even for unsafely-published instances. – Pavel Grushetzky Jun 02 '14 at 06:48

3 Answers3

4

Chapter 17 reasons about concurrent code in terms of happens before relationships. In your example, if you take two random threads then there is no happens-before relationship between this.value = initialValue; and result = value;.

So if you have something like:

  1. T1.start();
  2. T2.start();
  3. ...
  4. T1: L = new PlainSimpleAtomicLong(42);
  5. T2: long value = L.get();

The only happens-before (hb) relationships you have (apart from program order in each thread) is: 1 & 2 hb 3,4,5.

But 4 and 5 are not ordered. If however T1 called L.get() before T2 called L.get() (from a wall clock perspective) then you would have a hb relationship between unlock() in T1 and lock() in T2.

As already commented, I don't think your proposed code could break on any combination of JVM/hardware but it could break on a theoretical implementation of the JMM.

As for your suggestion to wrap the constructor in a lock/unlock, I don't think it would be enough because, in theory at least, T1 could release a valid reference (non null) to L before running the body of the constructor. So the risk would be that T2 could acquire the lock before T1 has acquired it in the constructor. There again, this is an interleaving that is probably impossible on current JVMs/hardware.

So to conclude, if you want theoretical thread safety, I don't think you can do without a volatile long value, which is how AtomicLong is implemented. volatile would guarantee that the field is initialised before the object is published. Note finally that the issues I mention here are not due to your object being unsafe (see @BrettOkken answer) but are based on a scenario where the object is not safely published across threads.

assylias
  • 321,522
  • 82
  • 660
  • 783
  • Nice! (and I'm learning the reasoning approach here). What about JLS 12.5, the "Just before a reference to the newly created object is returned as the result, the indicated constructor is processed ..." clause? Wouldn't it suggest that if the reference is visible, then the constructor had run? – Pavel Grushetzky Jun 02 '14 at 16:41
  • 12.5 describes the construction *in a given thread*. As soon as more than one thread is involved, you need to apply Chapter 17. See for example: http://stackoverflow.com/a/15517168/829571. I have also amended the last paragraph to make it clearer what threading issue I am addressing. – assylias Jun 02 '14 at 18:17
  • How does the fact that the read write lock is final and the value is only accessed /after/ the read write lock is obtained come into play? As covered in the post you reference, as of java 5 the final variable is guaranteed to be visible correctly. In the get method, the lock is obtained before the value variable is accessed. Obtaining the lock should guarantee that the correct value for value is read. As stated in the doc "A successful lock operation acts like a successful monitorEnter action" http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/locks/Lock.html – Brett Okken Jun 03 '14 at 00:51
  • @BrettOkken I think that theoretically, you could have the construction in T1 decomposed into (i) create new reference and allocate memory (ii) create and initialise final and volatile variables (iii) run the rest of the constructor body. If T2 reads the new reference due to (i) then the guarantee you mention means that T2 must also see (ii). But I believe it would be legal if T2 did not see (iii). Once again, that is an improbable interleaving/reordering. – assylias Jun 03 '14 at 00:56
  • @BrettOkken See [this post](http://stackoverflow.com/a/15517168/829571) that says something similar. – assylias Jun 03 '14 at 01:01
  • So then thread T2 could call get, have the read lock obtain a lock and access the value "value" before the constructor actually completes? So the key is to make sure the instance is cleanly initialized before being shared across threads. – Brett Okken Jun 03 '14 at 01:54
  • @BrettOkken yes that's the idea. If the instance is safely published then all these issues disappear. – assylias Jun 03 '14 at 01:56
  • As I think about this more, isn't the example you propose (modifying a non-volatile static variable without synchronization) going to have it's own set of race condition possibilities? If the static variable were volatile, wouldn't that enforce a "happens before" relationship? – Brett Okken Jun 03 '14 at 13:33
  • That would probably be enough yes. To be honest I still have doubts about the locking in the constructor - I'm not 100% sure that it can lead to a race although I fail to see why it would prevent it. – assylias Jun 03 '14 at 13:36
1

Assuming that you do not allow a reference to the instance to escape your constructor (your example looks fine), then a second thread can never see the object with any value of "value" other than what it was constructed with because all accesses are protected by a monitor (the read write lock) which was final in the constructor.

https://www.ibm.com/developerworks/library/j-jtp0618/ http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/locks/Lock.html

Brett Okken
  • 6,210
  • 1
  • 19
  • 25
  • 1
    That is necessary but I don't think that is sufficient. – assylias Jun 01 '14 at 21:22
  • it is sufficient as long as all access/mutation of the variable is done with correct protection of the read write mutex. – Brett Okken Jun 01 '14 at 23:03
  • I don't think so. Imagine a `public static PlainSimpleAtomicLong psal;` and one thread writing `psal = new PlainSimpleAtomicLong();`, then another thread calling `psal.get();` is not guaranteed to read 42. – assylias Jun 02 '14 at 06:23
  • that is an issue of references to the psal, not the behavior of the PlainSimpleAtomicLong class and the implementation he describes above. – Brett Okken Jun 02 '14 at 11:08
  • I think that's what the OP is asking about though. – assylias Jun 02 '14 at 12:53
-1

I think that for initial values , than both threads would see the same values (since they can have the object only after the constructor is finished).

But

If you change the value in 1 thread , then other thread may not see the same value if you don't use volatile

If you want to implement set, wrapping set with lock/unlock will not solve the problem - this is good when need atomic operation (like increment). I

It doesn't mean that it would work the way you want since you don't control the context switch. For example if 2 threads call set, with values 4 & 8 , since you don't know when the context switch occurs , you don't know who will gain the lock first.

Mzf
  • 5,210
  • 2
  • 24
  • 37
  • if you change the value in one thread (protected by write lock) then any other thread calling get (protected by read lock) will see the updated value. – Brett Okken Jun 01 '14 at 23:05