3

I am reading "Java Concurrency in practice" and looking at the example code on page 51.

This states that if a thread has references to a shared object then other threads may be able to access that object before the constructor has finished executing.

I have tried to put this into practice and so I wrote this code thinking that if I ran it enough times a RuntimeException("World is f*cked") would occur. But it isn't doing.

Is this a case of the Java spec not guaranting something but my particular implementation of java guaranteeing it for me? (java version: 1.5.0 on Ubuntu) Or have I misread something in the book?

Code: (I expect an exception but it is never thrown)

public class Threads {
 private Widgit w;

 public static void main(String[] s) throws Exception {
  while(true){
   Threads t = new Threads();
   t.runThreads();
  }
 }

 private void runThreads() throws Exception{
  new Checker().start();
  w = new Widgit((int)(Math.random() * 100)  + 1);
 }

 private class Checker extends Thread{
  private static final int LOOP_TIMES = 1000;

  public void run() {
   int count = 0;
   for(int i = 0; i < LOOP_TIMES; i++){
    try {
     w.checkMe();
     count++;
    } catch(NullPointerException npe){
     //ignore
    }
   }
   System.out.println("checked: "+count+" times out of "+LOOP_TIMES);
  }
 }

 private static class Widgit{
  private int n;
  private int n2;

  Widgit(int n) throws InterruptedException{
   this.n = n;
   Thread.sleep(2);
   this.n2 = n;
  }

  void checkMe(){
   if (n != n2) {
    throw new RuntimeException("World is f*cked");
   }
  }
 }

}
Gray
  • 115,027
  • 24
  • 293
  • 354
andy boot
  • 11,355
  • 3
  • 53
  • 66
  • i can't tell you about this specific problem, but one thing that's complicated about threading issues is that they are hard to reproduce. so even though your code isn't generating the error, that doesn't mean it isn't there. don't think the error will go away without being fixed. it won't. – zxcvbnm Apr 10 '10 at 16:29
  • What was the ouptut from your test? – Dominik Apr 10 '10 at 16:49
  • output was like this: checked: 652 times out of 1000 checked: 908 times out of 1000 checked: 753 times out of 1000 checked: 0 times out of 1000 checked: 695 times out of 1000 checked: 684 times out of 1000 checked: 0 times out of 1000 checked: 782 times out of 1000 checked: 475 times out of 1000 – andy boot Apr 12 '10 at 18:22
  • Firstly thanks for all your replies. I think I might have over-complicated things a little with my sample code. I have posted a new question which lead me down this path in the first place http://stackoverflow.com/questions/2624638/java-concurrency-in-practice-sample-question – andy boot Apr 12 '10 at 19:08

4 Answers4

3

You don't publish the reference until after the constructor has finished, change Widgit like this:

private class Widgit{ // NOTE: Not class is not static anymore
    private int n;
    private int n2;

    Widgit(int n) throws InterruptedException{
        this.n = n;
        w = this; // publish reference
        Thread.sleep(2);
        this.n2 = n;
    }

    void checkMe(){
        if (n != n2) {
        throw new RuntimeException("World is f*cked");
    }    
}

Should now throw.

Edit: You should also declare the Widgit field as volatile:

 private volatile Widgit w;
gustafc
  • 28,465
  • 7
  • 73
  • 99
  • Nice, but can you do it without publishing the reference in the constructor. That's what I'm really after and according to the book it is possible – andy boot Apr 13 '10 at 22:11
  • The JCiP example is about safe publication, not `this` references escaping prematurely. In that example (in `Holder.assertSanity`), the value for `n` is read twice. It would throw if the value is stale the first time it is read, and the memory is flushed before the second read so that the second read gets an up-to-date value. I would say it is a vary rare condition, and equally hard to manufacture. – gustafc Apr 14 '10 at 11:55
1

Well, you need to understand the issues a little more. It isn't really a case of anything being or not being "guaranteed." With concurrency problems, nothing is really guaranteed unless you really do specific things to force the problem to happen. You're just relying on the hope that enough runs should produce, which is not the case. These kinds of problems are hard to predict, which is why concurrency is a hard problem. You could try doing more work in your functions, but I assure you these are real problems that the runtime is not going to save you from.

BobbyShaftoe
  • 28,337
  • 7
  • 52
  • 74
1

Before sleeping, start a new thread which prints the value of n2. You will see the second thread can access the object before the constructor has finished.

The following example demonstrates this on the Sun JVM.

/* The following prints
Incomplete initialisation of A{n=1, n2=0}
After initialisation A{n=1, n2=2}
 */
public class A {
    final int n;
    final int n2;
    public A() throws InterruptedException {
        n = 1;
        new Thread(new Runnable() {
            public void run() {
                System.out.println("Incomplete initialisation of " + A.this);
            }
        }).start();
        Thread.sleep(200);
        this.n2 = 2;
    }
    @Override
    public String toString() {
        return "A{" + "n=" + n + ", n2=" + n2 + '}';
    }
    public static void main(String... args) throws InterruptedException {
        System.out.println("After initialisation " + new A());
    }
}
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
  • Nope. Either I get a NullPointerException or the constructor has initialized both values properly. (At least on my JVM) – andy boot Apr 12 '10 at 18:46
  • Not sure what you are doing to get an NPE. I have added an example to show you what I mean. – Peter Lawrey Apr 15 '10 at 21:06
  • Yes I can see that would work. But I dont want to start another thread inside the constructor. My concurrency book states that a thread started in the 'main' method of your code which has a reference to the original object can cause a failure. – andy boot Apr 16 '10 at 15:33
  • Not sure why you care. The thread just has to be started and a reference to that object needs to be packed up by the second thread before the constructor is completed. – Peter Lawrey Apr 16 '10 at 22:51
0

This will never throw a RunTimeException because your Widgit instance variable w remains null until the constructor code has executed. While your main thread is sleeping in the Widgit constructor, your Checker instance is hitting NullPointerException constantly as the w variable is still null. When your main thread finishes construction, the two int variables in Widgit are equal.

Finbarr
  • 31,350
  • 13
  • 63
  • 94