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).

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.

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!.

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.

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

Answer 5

One can write:

addWindowFocusListener(Singleton.this);

This will prevent NB from showing the warning.

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 {
  ...
  }
}

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();
      }
      ...
    }
}

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.

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.

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.

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