8

When I add an event listener using a lambda that calls an overridable method in the constructor, I get a warning. If I use a method reference, I don't get any warnings about overridable methods or leaking this. Should I avoid method references in the constructor or is it safe?

Here's a simple example:

public class SomeClass {

    public SomeClass(SomeObj obj) {
        obj.addListener(this::handleEvent); // no warnings, is it really safe?
        obj.addListener((event) -> handleEvent(event)); // warning about overridable method in constructor
    }

    private void handleEvent(Event event) {
        event.doSomething(someMethod());
    }

    private void someMethod() {
        ...
    }
}
PurkkaKoodari
  • 6,703
  • 6
  • 37
  • 58
Dave Wolfe
  • 660
  • 5
  • 14
  • I don't know for sure (hence the comment, not answer) -- but I can't see how it _would_ be safe. The closure has to include the `this` reference, and afaik there's no special consideration for that in lambdas. – yshavit Jul 01 '14 at 18:30
  • 1
    @SotiriosDelimanolis Why would that matter? The warnings are because of threading concerns and the possibility that `addListener` would invoke a method on the not-fully-constructed `this`. – yshavit Jul 01 '14 at 18:34
  • 2
    @yshavit The examples given are more or less equivalent. If the warning isn't given for one of them, it may just be a compiler feature missing (or vice-versa). – Sotirios Delimanolis Jul 01 '14 at 18:35
  • @SotiriosDelimanolis Sure, but that doesn't answer the question of whether it's safe. – yshavit Jul 01 '14 at 18:43
  • @yshavit Note that they asked the question assuming that the lambda was unsafe because they got a warning. If the method reference had a warning too they probably wouldn't have asked the question. Had they tried this on a different compiler, they may have seen the warning and not have had any doubts. Also, `the examples given are more or less equivalent` implies that if one isn't safe, neither is the other. – Sotirios Delimanolis Jul 01 '14 at 18:52

2 Answers2

8

No, this is most definitely not safe. You are publishing this where it may be accessed by alien code before the object has been fully constructed. Registering a listener from a constructor is always a no-no, whether you do it via an anonymous class or via a lambda or method reference. Your example is equivalent to the idiom I warned about in this article (12 years ago!): https://www.ibm.com/developerworks/library/j-jtp0618/

The involvement of method references here is a red herring; the problem is that you are making available a reference to a partially constructed object to code that you don't control.

The compiler can't warn you about everything; just because there's no warning doesn't mean your code is right :)

Brian Goetz
  • 90,105
  • 23
  • 150
  • 161
6

Method references and lambdas both evaluate to the same thing, a functional interface:

So, the two are essentially equivalent. In the case of the method reference, the target is explicitly this, and in the case of the lambda, it's implicitly this. So, their "warning-ness" is the same. The next question is: which one did the compiler get wrong? Is the warning wrong, or the lack of warning?

The reason the warning is there is that leaking this from the constructor has a couple of big dangers. One relates to multithreading: any memory visibility guarantees you get from final field semantics (as well as some other guarantees) are gone if you leak this from the reference. The other concern is that the addListener method will invoke the method on this right away, before the constructor has finished. That is, it'll be invoking a method on a partially-constructed object. This is especially problematic for overridable methods, because it could be that this constructor is for somebody else's superclass, and that somebody else has overridden the method in question. In that case, you'll be invoking the method on that subclass, whose constructor hasn't even had a chance to start yet (since a superclass's constructor is run first).

So, yes, the warning on the lambda is correct. And it should be there for the method reference, too.

yshavit
  • 42,327
  • 7
  • 87
  • 124