How can one break this (non?) thread safe object?

Question

I replied to a question earlier about thread safety which did not get a definite answer (I think).

So I have been trying to convince myself that the design is broken (visibility) by having thousands of threads read and write that object - but I have not been able to get anything unexpected. That is obviously not a proof that it is thread safe, probably merely a proof of my own limitations!

I understand the risk of reordering but I don't see how it could apply in that case, in the sense that the clone instance in the bar() method is local and the change on its fields is done before being released to the outside world with return, after which the instance is effectively immutable. So a thread looking at the returned object would see it with its bar field already set to the correct value...

So my question is: what kind of code Could you show a piece of code that uses IsItSafe and that could lead 2 threads to see different values of the bar field of a given instance of IsItSafe?

For reference and ease of reading I copy the code here:

public class IsItSafe implements Cloneable {

    private int foo;
    private int bar;

    public IsItSafe foo(int foo) {
        IsItSafe clone = clone();
        clone.foo = foo;
        return clone;
    }

    public IsItSafe bar(int bar) {
        IsItSafe clone = clone();
        clone.bar = bar;
        return clone;
    }

    public int getFoo() {
        return foo;
    }

    public int getBar() {
        return bar;
    }

    protected IsItSafe clone() {
        try {
            return (IsItSafe) super.clone();
        } catch (CloneNotSupportedException e) {
            throw new Error(e);
        }
    }
}

Answer 1

Visibility problems can occur when a value your program has written to a variable is cached in the cpu cache and not written to RAM immediately. For this reason if thread A running on cpu A is writting a value without correct synchronization and thread B reads this value from cpu B he might see a stale value in RAM and not the most recent value (present only in the cpu cache of processor A).

In the example given you are using none of the mechanisms Java provides to ensure safe publication. That is synchronization, volatile or final fields set in the constructor.

Therefore one could imagine that in your example the reference to the create clone object becomes availabe but the values written to clones fields remains in the cpu cache. In this case other threads would not be able to read the up to date values.

To give some reference. Look at this example

class FinalFieldExample {
  final int x;
  int y;
  static FinalFieldExample f;
  public FinalFieldExample() {
    x = 3;
    y = 4;
  }

  static void writer() {
    f = new FinalFieldExample();
  }

  static void reader() {
    if (f != null) {
      int i = f.x;
      int j = f.y;
    }
  }
}

The class above is an example of how final fields should be used. A thread executing reader is guaranteed to see the value 3 for f.x, because it is final. It is not guaranteed to see the value 4 for y, because it is not final.

The argument you are making would hold for this example as well, wouldn't it? The instance is created, fields set in the constructor etc. However it is not thread-safe, since the value written to y needs not become visible to other threads. (The cited example is from JSR 133 (Java Memory Model) FAQ: http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#reordering)

Update: You have asked for code that demonstrates the problem. I asked a similar (more open) question once: How to demonstrate java multithreading visibility problems? The interesting thing with the code sample given is, that it will behave differently with different minor versions of Java, on different operating systems and wether using the client or server jvm. In that regard I found the sample quite interesting. What is important to note is, that it might well be impossible to actually create sample code that leads to a visibility problem with your code today. However next year cpu generation might implement a different caching policy and suddenly the problem appears. If you follow the guidelines of the Java Language Specification your save.

Answer 2

Your code is irrelevant.

The problem is simply this: If bar is not volatile and if access is not synchronized, if one thread changes its value, another thread is not guaranteed to see the change.

Answer 3

I would worry more about bugs, like do you want foo to become 0 when you set bar?

Your fields are effectively final. Making them final and using a constructor to create new copies would make them thread safe. It would also make it more obvious that you are resetting foo when you change bar and visa-versa.

A problem you have is that you change the value of foo or bar (from 0 which is set in the constructor). This means if you look at this object in another thread, you might see the value you wanted or you might see a 0.

private IsItSafe iis = null;

// in thread A
iis = new IsItSafe().foo(123);

// in thread B
if (iis != null) {
   int foo = iis.getFoo();
   // foo could be 123, 
   // or it could be the initial value of the field foo which is 0.
}