9

Java concurrency in practice book has given an example for unsafe publication

public class Holder 
{
    private int n;
    public Holder(int n)
    {
        this.n = n; 
    }
    public void assertSanity() 
    {
        if (n != n)
            throw new AssertionError("This statement is false.");
    }
}

The above code seems to be thread safe. It would not be thread safe if n is public variable. Is the book example wrong?

Mr_and_Mrs_D
  • 32,208
  • 39
  • 178
  • 361
vinayag
  • 384
  • 1
  • 4
  • 13
  • 1
    None can modify anything as long as n is private, so your current code is indeed threadsafe. – rocketboy Sep 04 '13 at 16:00
  • Not considering reflection. – rocketboy Sep 04 '13 at 16:05
  • 1
    Basically, this used to be a serious potential problem, but the Java 5 spec made the memory model more rigorous, and it's no longer an issue. (`n` should still be `final`, though.) – chrylis -cautiouslyoptimistic- Sep 04 '13 at 16:05
  • A lot of incorrect comments on this page. It *is* possible that the object pointer is visible to other threads before the constructor is finished. – ZhongYu Sep 04 '13 at 16:24
  • 1
    @zhong.j.yu People keep saying that. I'd like to see an example please. I've yet to see one. They all assume that you already have a reference, without showing how the hell you got it. – Cruncher Sep 04 '13 at 16:38
  • 1
    @Cruncher VM can allocate a blank object, assign its address to a shared variable (that other threads can observe prematurely), *then* invoke the constructor. – ZhongYu Sep 04 '13 at 16:39
  • Also, if this is infact unsafe, can somebody show me how to make a thread safe then? – Cruncher Sep 04 '13 at 16:40
  • If I make the constructor to be private, and this class becomes thread safe, is it true? – vinayag Sep 04 '13 at 16:41
  • @Goms And how do you plan on making an instance of it with only a private constructor? You need a public static method to instantiate it then – Cruncher Sep 04 '13 at 16:41
  • Memory model does not care about private or not. – ZhongYu Sep 04 '13 at 16:42
  • Yeah, I forgot to mention that, obviously we need public static method. – vinayag Sep 04 '13 at 16:42

3 Answers3

10

Safe publication is about memory visibility. The concept of memory visibility is a bit trickier than other thread safety issues such as race conditions.

Memory visibility issues arise when actions performed by one thread in certain order appear to be performed in different order for another thread (it may be caused by optimizations made by compiler or CPU).

In your case:

// Thread A
h = new Holder(42);

// Thread B
h.assertSanity();

For Thread A, n is certainly initialized before h.

But in absense of safe publication it's not guaranteed to be the same for Thread B. Thread B may see h in initialized state, but n won't be initialzed yet. Moreover, the state of n as observed by Thread B may change during evaluation of n != n, causing assertSanity() to throw an exception.

Note that this problem is not something that is guaranteed to happen in all cases. You will probably never see this happening, but Java Memory Model doesn't guarantee correctness in this case nonetheless.

bluish
  • 26,356
  • 27
  • 122
  • 180
axtavt
  • 239,438
  • 41
  • 511
  • 482
  • +1 good explanation. Anybody know why the Java spec isn't changed to fix this? Basically, make constructors have an automatic happens before. What would that break? – user949300 Sep 04 '13 at 16:11
  • @user949300 requiring that an object reference is not assigned before the constructor is finished would be bad for optimization. – ZhongYu Sep 04 '13 at 16:17
  • 2
    I'm pretty certain (will need to look it up) that assignments made in a constructor are always visible to other threads. So `n` will always be initialised when observed from thread B – Gareth Davis Sep 04 '13 at 16:17
  • @user949300: The whole point of memory model is that any additional guarantees disallow certain optimization. As far as I know, on CPUs with relaxed memory models it may require insertion of memory barriers to each constructor, that may seriously harm performance. – axtavt Sep 04 '13 at 16:18
  • @axtavt (and zhong) However, in reality, this means that all classes that will ever be used in multithreaded environments (that's pretty much everywhere nowadays) need to be immutable (all finals), or have _all_ synchronized getters, which is also bad for optimization. Seems to me better to take the hit _once_ during construction and follow the principal of least astonishment and avoid these really obscure bugs. I'll do a little Googling. Maybe this should be a SO question in itself. (And maybe this was fixed in Java 6 as chrysalis claims?) – user949300 Sep 04 '13 at 16:29
  • @user949300 you only need to safe-publish the object pointer, that is a very reasonable thing to do. – ZhongYu Sep 04 '13 at 16:31
  • I still don't know where `Thread B` got `h` from. Can you show an example? – Cruncher Sep 04 '13 at 16:35
  • @user949300 however you are right that it is very counter-intuitive that the object pointer can be visible before the object is constructed. It leads to a lot of bugs (that at least are theoretically wrong). The memory model could be made stronger to prevent that; the cost is probably negligible. I heard that's what C# does. – ZhongYu Sep 04 '13 at 16:36
  • @Cruncher For example `h` is a public static variable (non-volatile) – ZhongYu Sep 04 '13 at 16:41
  • It is thread-safe barring unsafe publication:) – ZhongYu Sep 04 '13 at 16:44
  • 3
    +1 For this to happen, you would have to pass the object between without touching any read/write barriers. i.e. If you use a thread safe collection or a volatile field, this will not happen. – Peter Lawrey Sep 04 '13 at 16:45
  • @zhong.j.yu How do you make it safe against unsafe publication? – Cruncher Sep 04 '13 at 16:48
  • @Peter. Thanks, very useful info. So if I pass the object in a thread-safe Queue, to the other Thread, something I typically do, it will be o.k? How does that Queue interact with the Java Memory Model to enforce this? – user949300 Sep 04 '13 at 17:19
  • @user949300 When you, or a data structure you use, uses a read/write barrier, it is a barrier for all memory, not just the queue. – Peter Lawrey Sep 04 '13 at 17:22
2

It's not thread safe, because when the object of the class is created Holder.n is assigned a default value of 0.

Because of that, some threads can see the value 0 and others the value n that is passed to constructor.

Alexander Pogrebnyak
  • 44,836
  • 10
  • 105
  • 121
-1

This class is fine. We should usually expect that objects are shared between threads only through safe publication. In that case Holder.n will appear to be the same constant value for any observers.

Sharing objects through unsafe publication is sometimes justifiable, however the programmer must be responsible to make certain that the object can be used in that situation, e.g. String.

Most classes do not need to, and should not, be designed to work with unsafe publication.

ZhongYu
  • 19,446
  • 5
  • 33
  • 61
  • Could you please elaborate on when unsafe publication is something you want? – user949300 Sep 04 '13 at 16:17
  • hmm... I wouldn't. But I cannot rule out the possibility that it's a good thing to do in some narrow scenarios. – ZhongYu Sep 04 '13 at 16:20
  • You should rephrase "This class is fine *as long as it is safely published*" - I know it is what you meant but is not very clear. – assylias Sep 04 '13 at 17:07
  • @assylias my view is that safe publication can be assumed. by default we do not need to worry about unsafe publication. a class designed to cope with unsafe publication, e.g. String, must have its own particular reasons. "immutable class" is not as important as it's sold to be. – ZhongYu Sep 04 '13 at 17:21