views:

181

answers:

4

Referring to my earlier question on incompletely constructed objects, I have a second question. As Jon Skeet pointed out, there's an implicit memory barrier in the end of a constructor that makes sure that final fields are visible to all threads. But what if a constructor calls another constructor; is there such a memory barrier in the end of each of them, or only in the end of the one that got called in the first place? That is, when the "wrong" solution is:

public class ThisEscape {
    public ThisEscape(EventSource source) {
        source.registerListener(
            new EventListener() {
                public void onEvent(Event e) {
                    doSomething(e);
                }
            });
    }
}

And the correct one would be a factory method version:

public class SafeListener {
    private final EventListener listener;

    private SafeListener() {
        listener = new EventListener() {
            public void onEvent(Event e) {
                doSomething(e);
            }
        }
    }

    public static SafeListener newInstance(EventSource source) {
        SafeListener safe = new SafeListener();
        source.registerListener(safe.listener);
        return safe;
    }
}

Would the following work too, or not?

public class MyListener {
    private final EventListener listener;

    private MyListener() {
        listener = new EventListener() {
            public void onEvent(Event e) {
                doSomething(e);
            }
        }
    }

    public MyListener(EventSource source) {
        this();
        source.register(listener);
    }
}

Update: The essential question is that is this() guaranteed to actually call the private constructor above (in which case there would be the barrier where intended and everything would be safe), or is it possible that the private constructor gets inlined into the public one as an optimization to save one memory barrier (in which case there wouldn't be a barrier until in the end of the public constructor)?

Are the rules of this() defined precisely somewhere? If not, then I think we must assume that inlining chained constructors is allowed, and probably some JVMs or maybe even javacs are doing it.

+2  A: 

An object is considered to be completely initialized when its constructor finishes.

This applies also for chained constructors.

If you have to register in the constructor define the listener as a static inner class. This is safe.

Thomas Jung
Are you sure that the compiler doesn't detect that the private constructor is being called from another constructor, and then instead of actually getting called, it would get inlined into the calling (public) constructor, to save one memory barrier?
Joonas Pulakka
+1  A: 

EDIT After the comment that suggested the compiler inlining the private constructor (I had not thought of that optimization) chances are that the code will be unsafe. And the worst part of unsafe multithreaded code is that is seems to work, so you are better off avoiding it completely. If you want to play different tricks (you do really want to avoid the factory for some reason) consider adding a wrapper to guarantee the coherence of data in the internal implementation object and register in the external object.


My guess is that it will be fragile but ok. The compiler cannot know whether the internal constructor will be called only from within other constructors or not, so it has to make sure that the result would be correct for code calling only the internal constructor, so whatever mechanism it uses (memory barrier?) has to be in place there.

I would guess that the compiler would add the memory barrier at the end of each and every constructor. The problem is still there: you are passing the this reference to other code (possibly other threads) before it is fully constructed --that is bad--, but if the only ´construction´ that is left is registering the listener, then the object state is as stable as it will ever be.

The solution is fragile in that some other day, you or some other programmer may need to add another member to the object and may forget that the chained constructors is a concurrency trick and may decide to initialize the field in the public constructor, and in doing so will add a hard to detect potential data race in your application, so I would try to avoid that construct.

BTW: The guessed safety may be wrong. I don't know how complex/smart the compiler is, and whether the memory barrier (or the like) is something it could try to optimize away... since the constructor is private the compiler does have enough information to know that it is only called from other constructors, and that is enough information to determine that the synchronization mechanism is not necessary in the internal constructor...

David Rodríguez - dribeas
"The compiler cannot know whether the internal constructor will be called only from within other constructors or not, so it has to make sure that the result would be correct for code calling only the internal constructor." - Very good thinking, thanks! Unless, what if the compiler detects that the internal constructor is being called from another constructor, and then instead of actually calling the internal constructor, it would inline it into the public one... But hopefully there are no such optimizations :-)
Joonas Pulakka
There you have it: I did not think in the *inlining* of the constructor, but that is quite a simple optimization that most current compilers can actually perform. So the answer should be: don't do that.
David Rodríguez - dribeas
+1  A: 

Escaping object reference in c-tor can publish an incompletely constructed object. This is true even if the publication is the last statement in the constructor.

Your SafeListener might not behave ok in a concurrent environment, even if c-tor inlining is performed (which I think it's not - think about creating objects using reflection by accessing private c-tor).

Loop
"Your SafeListener might not behave ok in a concurrent environment, even if c-tor inlining is performed " - I think `SafeListener` will behave ok, it's how it should be done according to Java Concurrency in Practice. But `MyListener` certainly won't behave ok if inlining is performed. Also, even though the private constructor can be accessed "as is" via reflection, or via some other method of the enclosing class, it doesn't guarantee that that constructor wouldn't get inlined into other constructors (of the same class) calling it.
Joonas Pulakka
+1  A: 

Your second version is not correct, because it is allowing the 'this' reference to escape from the construction process. Having 'this' escape invalidates the initialization safety guarantees that give final fields their safety.

To address the implicit question, the barrier at the end of construction only happens at the very end of object construction. The intuition one reader offered about inlining is a useful one; from the perspective of the Java Memory Model, method boundaries do not exist.

Brian Goetz
I think you mean the *third* version (`MyListener`) is not correct? The second version is from JCIP, said to prevent `this` from escaping. But thanks! So calling `this()` is not a constructor call in the sense that it does **not** make the object fully constructed; the object is fully constructed only after the constructor, that got called in the first place, returns.
Joonas Pulakka
Hang on. 17.5.1 (in non-normative discussion) stats "Note that if one constructor invokes another constructor, and the invoked constructor sets a final field, the freeze for the final field takes place at the end of the invoked constructor." That would tend to indicate that the last example is safe. What implementations actually do is, of course, a completely different matter.
Tom Hawtin - tackline
@Tom: Wow! It indeed says that. Isn't that the official spec? So implementations should do it this way, or otherwise they're broken.
Joonas Pulakka