Why does my singleton class throw a StackOverflowerror?

Question

I have been writing a program. Everything is in program is controlled by the 'Engine' class. I have hence made it a singleton. Here is my current code that runs just fine.

    package org.bautista.cybersafe.core;

import javax.swing.SwingUtilities;

import org.bautista.cybersafe.ui.MainUI;
import org.bautista.cybersafe.util.Cache;
import org.bautista.cybersafe.util.Config;
import org.bautista.cybersafe.util.account.Account;
import org.bautista.cybersafe.util.account.AccountManager;
import org.bautista.cybersafe.util.user.User;
import org.bautista.cybersafe.util.user.UserManager;

public class Engine {
    private static Engine instance;
    private AccountManager accountManager;
    private final MainUI ui;
    private final UserManager userManager;
    private final Config config;
    private User currentUser;

    private Engine() {
        instance = this; //THIS IS LINE 22
        if (!Cache.cacheExists()) {
            if (!Cache.createCache()) {
                System.out.println("Error creating cache.");
            }
        }
        config = new Config();
        userManager = new UserManager();
        ui = new MainUI();
    }

    public static Engine getInstance() {
        return instance == null ? instance = new Engine() : instance;
    }

    public void setCurrentUser(User user) {
        currentUser = user;
    }

    public User getCurrentUser() {
        return currentUser;
    }

    public AccountManager getAccountManager() {
        return accountManager;
    }

    public Config getConfig() {
        return config;
    }

    public UserManager getUserManager() {
        return userManager;
    }

    public void logOut() {
        currentUser = null;
        accountManager = null;
        ui.showLogin();
    }

    public void openAccountViewer(final Account account) {
        ui.showAccount(account);
        ui.setTitle("Cyber Safe - [" + currentUser.getUsername() + "] -"
                + account.getName());
    }

    public void openCreateAccountScreen() {
        ui.showCreateAccount();
    }

    public void openCreateUserScreen() {
        ui.showCreateUser();
    }

    public void openLoginScreen() {
        ui.showLogin();
        ui.setTitle("Cyber Safe");
    }

    public void openSafeScreen() {
        if (accountManager == null) {
            accountManager = new AccountManager(currentUser);
        }
        ui.showSafe();
        ui.setTitle("Cyber Safe - [" + currentUser.getUsername() + "]");
    }

    public void refreshUI() {
        ui.refresh();
    }

    public void updateAccountPreviews() {
        ui.updateAccountScroller();
    }

    public void run() {
        try {
            SwingUtilities.invokeAndWait(() -> ui.setVisible(true));
        } catch (final Exception e) {
            e.printStackTrace();
        }
    }

}

When I comment out line 22

instance = this;

I receive a StackOverflowerror. When I debug the program I find that the Engine constructor is being called repeatedly, as if it were performing recursion, until I final get the error. Why does that happen? Should't my #getInstance() method be initiating instance as a new instance of the 'Engine' class?

Here is the stacktrace:

Exception in thread "main" java.lang.StackOverflowError
at java.io.InputStream.<init>(InputStream.java:45)
at java.io.FileInputStream.<init>(FileInputStream.java:123)
at org.bautista.cybersafe.util.Config.loadProperties(Config.java:67)
at org.bautista.cybersafe.util.Config.<init>(Config.java:29)
at org.bautista.cybersafe.core.Engine.<init>(Engine.java:28)
at org.bautista.cybersafe.core.Engine.getInstance(Engine.java:34)
at org.bautista.cybersafe.util.user.UserManager.loadUsers(UserManager.java:73)
at org.bautista.cybersafe.util.user.UserManager.<init>(UserManager.java:20)
at org.bautista.cybersafe.core.Engine.<init>(Engine.java:29)
at org.bautista.cybersafe.core.Engine.getInstance(Engine.java:34)
at org.bautista.cybersafe.util.user.UserManager.loadUsers(UserManager.java:73)
at org.bautista.cybersafe.util.user.UserManager.<init>(UserManager.java:20)
at org.bautista.cybersafe.core.Engine.<init>(Engine.java:29)
at org.bautista.cybersafe.core.Engine.getInstance(Engine.java:34)
at org.bautista.cybersafe.util.user.UserManager.loadUsers(UserManager.java:73)
at org.bautista.cybersafe.util.user.UserManager.<init>(UserManager.java:20)
at org.bautista.cybersafe.core.Engine.<init>(Engine.java:29)
at org.bautista.cybersafe.core.Engine.getInstance(Engine.java:34)
at org.bautista.cybersafe.util.user.UserManager.loadUsers(UserManager.java:73)
at org.bautista.cybersafe.util.user.UserManager.<init>(UserManager.java:20)
at org.bautista.cybersafe.core.Engine.<init>(Engine.java:29)

And here is the full project on Github

Thanks in advance!

Answer 1

The stack trace shows the following loop:

at org.bautista.cybersafe.util.user.UserManager.loadUsers(UserManager.java:73)
at org.bautista.cybersafe.util.user.UserManager.<init>(UserManager.java:20)
at org.bautista.cybersafe.core.Engine.<init>(Engine.java:29)
at org.bautista.cybersafe.core.Engine.getInstance(Engine.java:34)

Engine.getInstance() calls new Engine().
new Engine() calls new UserManager().
new UserManager() calls UserManager.loadUsers().
UserManager.loadUsers() calls Engine.getInstance(), but Engine.instance hasn't been assigned yet, since the previous new Engine() call hasn't returned yet.

This is why assigning Engine.instance in the constructor, before it calls new UserManager(), fixes the problem.

You should reorganize your code to prevent that initialization loop. UserManager and Engine should not be co-dependent during initialization.

Note that doing private static Engine instance = new Engine() as suggested in another answer will not fix your initialization loop.