4

Let's see this simple Java program:

import java.util.*;

class A {
    static B b;
    static class B {
        int x;
        B(int x) {
            this.x = x;
        }
    }
    public static void main(String[] args) {
        new Thread() {
            void f(B q) {
                int x = q.x;
                if (x != 1) {
                    System.out.println(x);
                    System.exit(1);
                }
            }
            @Override
            public void run() {
                while (b == null);
                while (true) f(b);
            }
        }.start();
        for (int x = 0;;x++)
            b = new B(Math.max(x%2,1));
    }
}

Main thread

The main thread creates an instance of B with x set to 1, then writes that instance to the static field A.b. It repeats this action forever.

Polling thread

The spawned thread polls until it finds that A.b.x is not 1.

?!?

Half the time it goes in an infinite loop as expected, but half the time I get this output:

$ java A
0

Why is the polling thread able to see a B that has x not set to 1?

x%2 instead of just x is here simply because the issue is reproducible with it.

I'm running openjdk 6 on linux x64.

Dog
  • 7,707
  • 8
  • 40
  • 74
  • Are you saying you *can't* replicate this result without the `synchronized (this) {}`, or using `Math.max(x,1)`? – T.J. Crowder Apr 23 '13 at 20:04
  • You've tried each of those independently? – T.J. Crowder Apr 23 '13 at 20:05
  • In class B, make the instance variable `final` and see what happens. You're observing the value of `b.x` in between its creation and its being set to the value passed into the constructor. – David Conrad Apr 23 '13 at 20:08
  • 1
    @DavidConrad: But he shouldn't be. The assignment to the static `b` shouldn't be happening until after the constructor finishes. – T.J. Crowder Apr 23 '13 at 20:09
  • Ah, right. It's a visibility issue. The updated value of x isn't visible in the other thread. One of the guarantees of `final`, though, is that the value will be visible to any other thread. – David Conrad Apr 23 '13 at 20:12
  • @T.J.Crowder: okay renamed. Also it turns out you can remove the synchronized block. It must have been the last change I made before posting the question that enabled me to remove it. – Dog Apr 23 '13 at 20:14

3 Answers3

10

Here is what I think: because b is not final, the compiler is free to reorder the operations as it likes, right? So this, fundamentally is a reordering issue and as a result a unsafe publication issue Marking the variable as final will fix the problem.

More or less, it is the same example as provided here in the Java memory model docs.

The real question is how is this possible. I can also speculate here (since I have no idea how the compiler will reorder), but maybe the reference to B is written to the main memory (where it is visible to the other thread) BEFORE the write to x happens. In between these two operations the read happens, thus the zero value

Gray
  • 115,027
  • 24
  • 293
  • 354
Eugene
  • 117,005
  • 15
  • 201
  • 306
  • The example you linked to explains it perfectly. Implementation details are uninteresting and complicated. – Dog Apr 24 '13 at 14:05
1

Often the considerations surrounding concurrency focus on erroneous changes to state or on deadlocks. But visibility of state from different threads is equally important. There are many places in a modern computer where state can be cached. In registers, L1 cache on the processor, L2 cache between the processor and memory, etc. JIT compilers and the Java memory model are designed to take advantage of caching whenever possible or legal, because it can speed things up.

It can also give unexpected and counterintuitive results. I believe that is happening in this case.

When an instance of B is created, the instance variable x is briefly set to 0 before being set to whatever value was passed to the constructor. In this case, 1. If another thread tries to read the value of x, it could see the value 0 even if x has already been set to 1. It could be seeing a stale, cached value.

To ensure that the up-to-date value of x is seen, there are several things you can do. You could make x volatile, or you could protect the read of x with synchronization on the B instance (for example, by adding a synchronized getX() method). You could even change x from an int to a java.util.concurrent.atomic.AtomicInteger.

But by far the simplest way to correct the problem is to make x final. It is never going to change during the lifetime of B anyway. Java makes special guarantees for final fields, and one of them is that after the constructor completes, a final field set in the constructor will be visible to any other thread. That is, no other thread will see a stale value for that field.

Making fields immutable has many other benefits as well, but this is a great one.

See also Atomicity, Visibility, and Ordering by Jeremy Manson. Particularly the part where he says:

(Note: when I say synchronization in this post, I don't actually mean locking. I mean anything that guarantees visibility or ordering in Java. This can include final and volatile fields, as well as class initialization and thread starts and joins and all sorts of other good stuff.)
David Conrad
  • 15,432
  • 2
  • 42
  • 54
  • "When an instance of B is created, the instance variable x is briefly set to 0 before being set to whatever value was passed to the constructor.[...] It could be seeing a stale, cached value." Why? – Dog Apr 23 '13 at 20:56
  • In `f`, if I change `if (x != 1) {` to `if (q.x != 1) {` and `System.out.println(x);` to `System.out.println(q.x);`, the console output is either 1 or the program never halts. http://pastebin.com/8AwtyMAt – Dog Apr 23 '13 at 21:13
  • If for whatever reason it is semantically impossible to make a particular field `final`, but one wants to ensure that effects of a write to that field in the constructor will be visible before anything the constructor's caller might do with the new object, what's the best way to impose the appropriate happens-before relationship? – supercat Jan 24 '14 at 20:00
  • I think you would have to use `synchronized` or `volatile` in that case, but I'm not certain. – David Conrad Jan 27 '14 at 17:11
0

It seems to me that there might be a race condition on the B.x, such that there could exist a split second where B.x has been created and B.x=0 prior to the this.x = x in B's constructor. The series of events would be something like:

B is created (x defaults to 0) -> Constructor is ran -> this.x = x

Your thread is accessing B.x sometime after it was created but before the constructor ran. I was unable to recreate the problem locally, however.

CodeChimp
  • 8,016
  • 5
  • 41
  • 79
  • 2
    **How?** The constructor is called, and only once it's done is the resulting instance assigned to the static `b`. You're saying that the result of `new B(...)` is assigned to the static `b` before the constructor runs. That would be extraordinary, and extraordinary claims require extraordinary evidence (or in this case, references). :-) – T.J. Crowder Apr 23 '13 at 20:09
  • 2
    No, I was thinking this too, at first. What happens is, the object is allocated, the constructor is called, x is 0, that value is cached in some register or CPU cache line, x is set to 1, the constructor finishes, the other thread tries to read x, and gets the stale, cached value. Making x final, volatile, an AtomicInteger, or protecting the read with synchronization on the instance of B will correct it. – David Conrad Apr 23 '13 at 20:15
  • The question is: What happens first, the assignment or the constructor? I doubt this is a "leak", so the only explanation I can find, albeit against my intuition, is that the constructor is happening after the assignment. Perhaps its some optimization the JVM tried to make. – CodeChimp Apr 23 '13 at 20:17
  • @DavidConrad: That at least has some logic to it. I don't like it and find it very hard to believe (I don't currently have a system where I can try it, I'm on the road), but there's some logic there... – T.J. Crowder Apr 23 '13 at 20:18
  • 1
    I have been unable to recreate it, so I can only make the best educated guess based on what the OP says he has observed. I don't like it, either, as it now makes me question 14yrs of Java code I have written. – CodeChimp Apr 23 '13 at 20:19
  • Here's a thought...since B is static, it's a shared memory location. As soon as you run the constructor, that updates the shared memory (default of x is 0). Prior to this.x being set, the Thread accesses B.x, which has now been set to 0 (again, it's a shared, static instance). Whalla (or 'voilà'), race condition... – CodeChimp Apr 23 '13 at 20:22
  • 'Whalla' is also spelled voilà. :) – David Conrad Apr 23 '13 at 20:25
  • @DavidConrad, why do you think of caching? I'm starting to think it's simply because the `A.b` is written to a new `B` before that new `B`'s `x` is set... however ridiculous that seems. – Dog Apr 23 '13 at 20:34
  • I don't believe A.b will be set before the constructor finishes. However, visibility problems between threads when there is no synchronization are a well known problem. Have you tried what I suggested, making x final? What was the result? – David Conrad Apr 23 '13 at 20:39
  • @DavidConrad, yes making X final fixes it, because of the final field semantics part of JLS. but of course that's just one of the million ways to fix it. – Dog Apr 23 '13 at 20:52
  • We are all assuming that the creation of the new B (and thus the new B.x) happen as one stroke. I think the point that is being missed is that the actual machine code that is generated is not that simple. It will result in several steps in an order we probably can't predict by looking at the high-level language instructions. Since this is a threaded application, you don't know at which point the second thread will attempt to access B.x. And since the access, both read and write, to B.x is not sync'ed at all you cannot say that the read doesn't happen 1/2 during the write. – CodeChimp Apr 24 '13 at 11:51