Leaking this in constructor warning

Question

I'd like to avoid (most of the) warnings of Netbeans 6.9.1, and I have a problem with the 'Leaking this in constructor' warning.

I understand the problem, calling a method in the constructor and passing "this" is dangerous, since "this" may not have been fully initialized.

It was easy to fix the warning in my singleton classes, because the constructor is private and only called from the same class.

Old code (simplified):

private Singleton() {
  ...
  addWindowFocusListener(this);
}

public static Singleton getInstance() {

  ...
  instance = new Singleton();
  ...
}

New code (simplified):

private Singleton() {
  ...
}

public static Singleton getInstance() {

  ...
  instance = new Singleton();
  addWindowFocusListener( instance );
  ...
}

This fix is not working if the constructor is public and can be called from other classes. How is it possible to fix the following code:

public class MyClass {

  ...
  List<MyClass> instances = new ArrayList<MyClass>();
  ...

  public MyClass() {
    ...
    instances.add(this);
  }

}

Of course I want a fix which does not require to modify all my code using this class (by calling an init method for instance).

Neutral

Not related to the question directly but why does MyClass have a List of itself. Even if this relationship is justified in many cases, why does it add it's own self in the List. Imagine the resulting data structure in memory. – Naranjo

@CKing, my guess is the OP omitted static from the instances variable in his typing of the example. – Apothecium

Use a static creation method. – Breakfast

Answer 1

Since you make sure to put your instances.add(this) at the end of the constructor you should IMHO be safe to tell the compiler to simply suppress the warning (*). A warning, by its nature, doesn't necessarily mean that there's something wrong, it just requires your attention.

If you know what you're doing you can use a @SuppressWarnings annotation. Like Terrel mentioned in his comments, the following annotation does it as of NetBeans 6.9.1:

@SuppressWarnings("LeakingThisInConstructor")

(*) Update: As Isthar and Sergey pointed out there are cases where "leaking" constructor code can look perfectly safe (as in your question) and yet it is not. Are there more readers that can approve this? I am considering deleting this answer for the mentioned reasons.

Jacob

This is not a standard java warning, this is a Netbeans-warning. So adding @SuppressWarnings does not help. – Neutral

@asalamon: I see. Tant pis. And netbeans in turn, doesn't it provide any option to turn off specific warnings? Eclipse does. – Jacob

@asalamon: Got it. And would netbeans even ignore the @SuppressWarning("all")? For depending on the interpretation "all" could include the netbeans warnings as well. Sorry for not being more precise, I never worked with netbeans... – Jacob

@Terrel: Thanks. Have incorporated that in the answer. Do you know whether NetBeans does account for these annotations? – Jacob

@chiccodoro: NetBeans (6.9.1) suggested it, so NetBeans is happy. Eclipse, however, doesn't like it, so you're stuck with a warning in one place or the other. – Diesel

I've just tried the new spelling and it works correctly. Thanks Terrel. – Neutral

@grom: Thanks for the fix, didn't notice that – Jacob

Not sure about 6.9.1 but I have 7.0.1 and can customize warnings and errors. See Preferences > Editor > Hints. – Fastback

This is only 'safe' in a single-threaded setting. From JLS 7 17.5: "An object is considered to be completely initialized when its constructor finishes. A thread that can only see a reference to an object after that object has been completely initialized is guaranteed to see the correctly initialized values for that object's final fields". No such guarantee if you leak this to another thread in the constructor! Also 'at the end of the constructor' may be reordered by the VM. – Maclay

@Maclay - these are strong points! I have two questions to better understand them: ad 1 - how would you pass this to a different thread from inside the constructor? ad 2 - could the VM possibly reorder lines of code in the constructor even if the reordering changed the semantics of the code? - I suggest you right a new answer to this since it differs pretty much from the other things said so far, and it would give you more freedom to elaborate ;-) – Jacob

@Jacob - I added an answer. I hope it answers your two questions.. (The reordering is in the example specific to final fields only. However the VM may reorder quite a bit see for example table 17.5 in the JLS 7.) If I left things unclear, please comment my answer! – Maclay

The JLS §17.5 is about "final" field semantics specifically. But if you don't use any final fields, what's the risk with multithreading? As long as the execution order is as programmed, you control who gets the handle and when. - Is there some aspect I missed? – Gromwell

@foo, I just tried hard to rephrase my answer to incorporate your remarks properly, but I feel that I am too much trying to explain things I have not dived into enough to fully understand the details. I simlpy cut down my text to refer to the comments and Ishtar's answer. Maybe you can look through that answer and help improve it if you find a missing point? Especially I realized that the (methinks) most important point of "may be reordered by the VM" does not have a reference, the JLS 7 17.5 is about guarantees that are made but not explicit about guarantees that are not made. – Jacob

What if some other class extends this class? Then even if this is leaked at the end of the constructor, it's still not at the end of the construction process, right? Which would mean that it's probably not safe even in a single-threaded environment. That is, unless the class is final or its constructor is private. – Stencil

Suppressing a warning which is caused by a design flaw isn't recommended – Sy

@abdulrahmank, you are so right. You may want to expand on why the given design is a "design flaw" though so your comment can really help anybody. – Jacob

I would humbly like to bring light to the warning "leaking this in constructor" is caused because we may start using an object even before its completely constructed. And also by design flaw i mean having a class to manage its own object creation and management , we are violating abstraction hence we may need to have an ObjectManager class to manage the class's object. Having to track all the objects of own's one object will obviously violate abstraction. Please correct me if I'm wrong @Jacob – Sy

Answer 2

[Remark by chiccodoro: An explanation why/when leaking this can cause issues, even if the leaking statement is placed last in the constructor:]

Final field semantics is different from 'normal' field semantics. An example,

We play a network game. Lets make a Game object retrieving data from the network and a Player object that Listens to events from the game to act accordingly. The game object hides all the network details, the player is only interested in events:

import java.util.*;
import java.util.concurrent.Executors;

public class FinalSemantics {

    public interface Listener {
        public void someEvent();
    }

    public static class Player implements Listener {
        final String name;

        public Player(Game game) {
            name = "Player "+System.currentTimeMillis();
            game.addListener(this);//Warning leaking 'this'!
        }

        @Override
        public void someEvent() {
            System.out.println(name+" sees event!");
        }
    }

    public static class Game {
        private List<Listener> listeners;

        public Game() {
            listeners = new ArrayList<Listener>();
        }

        public void start() {
            Executors.newFixedThreadPool(1).execute(new Runnable(){

                @Override
                public void run() {
                    for(;;) {
                        try {
                            //Listen to game server over network
                            Thread.sleep(1000); //<- think blocking read

                            synchronized (Game.this) {
                                for (Listener l : listeners) {
                                    l.someEvent();
                                }
                            }
                        } catch (InterruptedException e) {
                            e.printStackTrace();
                        }
                    }
                }            
            });
        }

        public synchronized void addListener(Listener l) {
            listeners.add(l);
        }
    }

    public static void main(String[] args) throws InterruptedException {
        Game game = new Game();
        game.start();
        Thread.sleep(1000);
        //Someone joins the game
        new Player(game);
    }
}
//Code runs, won't terminate and will probably never show the flaw.

Seems all good: access to the list is correctly synchronized. The flaw is that this example leaks the Player.this to Game, which is running a thread.

Final is quite scary:

...compilers have a great deal of freedom to move reads of final fields across synchronization barriers...

This pretty much defeats all proper synchronizing. But fortunately

A thread that can only see a reference to an object after that object has been completely initialized is guaranteed to see the correctly initialized values for that object's final fields.

In the example, the constructor writes the objects reference to the list. (And thus has not been completely initialized yet, since the constructor did not finish.) After the write, the constructor is still not done. It just has to return from the constructor, but let's assume it hasn't yet. Now the executor could do its job and broadcast events to all the listeners, including the not yet initialized player object! The final field of the player (name) may not be written, and will result in printing null sees event!.

Maclay

As mentioned in our discussion to my answer: A very strong point! Ishtar's answer might be slightly easier to understand after having read his initial comment "This is only 'safe' in a single-threaded setting. (...) No such guarantee if you leak this to another thread in the constructor! Also 'at the end of the constructor' may be reordered by the VM." – Jacob

BTW - maybe the problem of the OP is a design issue. The fact that an object registers itself somewhere when being created could indicate that its constructor does something it is not meant for: The constructor should initialize the instance and not modify any other objects. – Jacob

I just got this error, but realized one class that does this is wrapped in an init method, that is called by the constructor. @Maclay does this do anything to help this "leaking issue?" there are no more warnings, so i think i am okay? – Frazer

@XaolingBao, please create a new question with more details about your problem. – Maclay

It gets worse if you ever subclass the Player class - then the subclass's fields will not have been initialised before the final step in Player's constructor, and you have a potentially very badly broken object being leaked (this is an issue even in a single-threaded setting!). – Licentiate

Answer 3

The best options you have :

• Extract your WindowFocusListener part in another class (could also be inner or anonymous) . The best solution, this way each class has a specific purpose.
• Ignore the warning message.

Using a singleton as a workaround for a leaky constructor is not really efficient.

Glaswegian

@asalamon74, but I suppose that the second wasn't. Wanting a singleton with a public constructor is a non-sense, so I guessed you tried to apply the singleton pattern as a workaround. – Glaswegian

Of course I don't want a Singleton with a public constructor. I just wanted to show an example where I was able to fix the problem (this was the Singleton) and a different example where I was unable to fix it (this is the non-Singleton example). – Neutral

@asalamon74, Ho, then I didn't understood how you got to the singleton solution. Anyway, as I said in my answer, you should extract your listener, that's the best way. – Glaswegian

Reading all these again, I'd prefer this answer over mine. For the second bullet point, however, I think that adding a @SuppressWarnings is better than ignoring the warnings. You know, broken windows... if you have 17 warnings in your code, you don't remember how many and which of them you want to ignore. – Jacob

This is the answer. A good Explanation and this will keep "The Single Responsibility Principle" which is the first rule of the OOP. – Frock

Answer 4

This is a good case of where a Factory that created instances of your class would helpful. If a Factory was responsible for creating instances of your class, then you would have a centralized location where the constructor is called, and it would be trivial to add a required init() method to your code.

Regarding your immediate solution, I would suggest that you move any calls that leak this to the last line of your constructor, and then suppress them with an annotation once you've "proved" that it is safe to do so.

In IntelliJ IDEA, you can suppress this warning with the following comment right above the line:
//noinspection ThisEscapedInObjectConstruction

Quinacrine

Answer 5

One can write:

addWindowFocusListener(Singleton.this);

This will prevent NB from showing the warning.

Holism

Answer 6

Using a nested class (as suggested by Colin) is probably your best option. Here's the pseudocode:

private Singleton() {
  ...
}

public static Singleton getInstance() {

  ...
  instance = new Singleton();
  addWindowFocusListener( new MyListener() );
  ...

  private class MyListener implements WindowFocusListener {
  ...
  }
}

Lilylivered

Answer 7

There is no need of separate listener class.

public class Singleton implements WindowFocusListener {

    private Singleton() {
      ...
    }    

    private void init() {
      addWindowFocusListener(this);
    }

    public static Singleton getInstance() {    
      ...
      if(instance != null) {
        instance = new Singleton();
        instance.init();
      }
      ...
    }
}

Avram

Maybe in GUI programming this is okay but in general this code isn't thread-safe. – Kaenel

You could just move the construction and call to init() to the static initialiser, I guess, and assign to the static final field after calling init(). – Goldina

Answer 8

The annotation @SuppressWarnings("LeakingThisInConstructor") applicable only to the class an not to the constructor itself.

Solution I would suggest: create private method init(){/* use this here*/} and call it from the constructor. The NetBeans won't warn you.

Anubis

This isn't a solution. This is avoiding the real problem. – Cloddish

@Cloddish - honestly, the same holds for my answer. Re-reading all these answers, I think that Colin's answer is the best. – Jacob

Answer 9

The error "Leaking this in constructor" is one of the bugs that are really annoying and hard to tell if it is actually a problem. Test cases will pass most of the time and they might even be setup in a way that they pass always and the error only occurs on production.

Having such code with Listeners in the UI is quite common and they usually don't fail because of the single UI thread that makes everything easier, but sometimes not. If you create an object within the UI thread and this object adds some listeners within the UI thread, it is very likely that those listeners are called also within the UI thread since they react to UI events. If you are sure about the setup there, you may just ignore the error.

On the other hand when dealing with multi-threaded applications and classes that for example handle some IO operations and background work, this issue can cause bugs that are hard to detect and usually occur on the weekend when the admin is on holidays and the presentation on the next day includes the biggest clients of the company.

Here is how I would fix the code:

public class MyClass {

    private final List<MyClass> instances = new ArrayList<MyClass>();

    public static MyClass create() {
        MyClass mc = new MyClass();
        mc.init();
        return mc;
    }

    /** private constructor. */
    private MyClass() {}

    private void init() {
        instances.add(this);
    }

}

Provide a factory method or create a separate factory class that is able to create your objects. This factory is responsible to call the init() method after the object has been created.

This requires you to search for references where the constructor is used and update to the new method. As a refactoring step in between I suggest to deprecate the constructor so that you can update the clients to use the new factory method before it is changed, e.g.:

public class MyClass {

    private final List<MyClass> instances = new ArrayList<MyClass>();

    public static MyClass create() {
        MyClass mc = new MyClass();
        // mc.init(); // will be called when the constructor call will be removed
        return mc;
    }

    /** @deprecated use {@link #create()} instead. */
    @Deprecated
    public MyClass() {
        init();
    }

    private void init() {
        instances.add(this);
    }

}

An alternative would be to simply make init() public and force your clients to call it. This gives clients control over the lifecycle of created objects and I would even prefer to do so in cases where init() has to do some actual work, like open or use a connection.

Maros

Answer 10

Say you originally had a class like this that used itself as an ActionListener and therefore you end up calling addActionListener(this) which generates the warning.

private class CloseWindow extends JFrame implements ActionListener {
    public CloseWindow(String e) {
        setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
        setLayout(new BorderLayout());

        JButton exitButton = new JButton("Close");
        exitButton.addActionListener(this);
        add(exitButton, BorderLayout.SOUTH);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        String actionCommand = e.getActionCommand();

        if(actionCommand.equals("Close")) {
            dispose();
        }
    }
}

As @Colin Hebert mentioned, you could separate the ActionListener out into its own class. Of course this would then require a reference to the JFrame that you want to call .dispose() on. If you'd prefer not to fill up your variable name space, and you want to be able to use the ActionListener for multiple JFrames, you could do it with getSource() to retrieve the button followed by a chain of getParent() calls to retrieve the Class that extends JFrame and then call getSuperclass to make sure it's a JFrame.

private class CloseWindow extends JFrame {
    public CloseWindow(String e) {
        setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
        setLayout(new BorderLayout());

        JButton exitButton = new JButton("Close");
        exitButton.addActionListener(new ExitListener());
        add(exitButton, BorderLayout.SOUTH);
    }
}

private class ExitListener implements ActionListener {
    @Override
    public void actionPerformed(ActionEvent e) {
        String actionCommand = e.getActionCommand();
        JButton sourceButton = (JButton)e.getSource();
        Component frameCheck = sourceButton;
        int i = 0;            
        String frameTest = "null";
        Class<?> c;
        while(!frameTest.equals("javax.swing.JFrame")) {
            frameCheck = frameCheck.getParent();
            c = frameCheck.getClass();
            frameTest = c.getSuperclass().getName().toString();
        }
        JFrame frame = (JFrame)frameCheck;

        if(actionCommand.equals("Close")) {
            frame.dispose();
        }
    }
}

The above code will work for any button that is a child at any level of a class which extends JFrame. Obviously if your object just is a JFrame it's just a matter of checking that class directly rather than checking the super class.

Ultimately using this method you're getting a reference to something like this: MainClass$CloseWindow which has the super class JFrame and then you're casting that reference to JFrame and disposing of it.

Alonsoalonzo

instead of frameTest.equals("javax.swing.JFrame") simply use if (frameCheck instanceof JFrame) {..} – Maros

Answer 11

Wrap your this in double brackets. Netbeans ignores some errors by default if they are in sub-statements.

  public MyClass() {
     ...
     instances.add((this));
  }

https://stackoverflow.com/a/8357990

Lizalizabeth