Why does this simple Java Swing program freeze?

Question

Below is a simple Java Swing program that consists of two files:

• Game.java
• GraphicalUserInterface.java

The graphical user interface displays a "New Game" button, followed by three other buttons numbered 1 to 3.

If the user clicks on one of the numbered buttons, the game prints out the corresponding number onto the console. However, if the user clicks on the "New Game" button, the program freezes.

(1) Why does the program freeze?

(2) How can the program be rewritten to fix the problem?

(3) How can the program be better written in general?

Source

Game.java:

public class Game {

    private GraphicalUserInterface userInterface;

    public Game() {
        userInterface = new GraphicalUserInterface(this);
    }

    public void play() {
        int selection = 0;

        while (selection == 0) {
            selection = userInterface.getSelection();
        }

        System.out.println(selection);
    }

    public static void main(String[] args) {
        Game game = new Game();
        game.play();
    }

}

---

GraphicalUserInterface.java:

import java.awt.BorderLayout;

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;

public class GraphicalUserInterface extends JFrame implements ActionListener {

    private Game game;
    private JButton newGameButton = new JButton("New Game");
    private JButton[] numberedButtons = new JButton[3];
    private JPanel southPanel = new JPanel();
    private int selection;
    private boolean isItUsersTurn = false;
    private boolean didUserMakeSelection = false;

    public GraphicalUserInterface(Game game) {
        this.game = game;

        newGameButton.addActionListener(this);

        for (int i = 0; i < 3; i++) {
            numberedButtons[i] = new JButton((new Integer(i+1)).toString());
            numberedButtons[i].addActionListener(this);
            southPanel.add(numberedButtons[i]);
        }

        getContentPane().add(newGameButton, BorderLayout.NORTH);
        getContentPane().add(southPanel, BorderLayout.SOUTH);

        pack();
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setLocationRelativeTo(null);
        setVisible(true);
    }

    public void actionPerformed(ActionEvent event) {
        JButton pressedButton = (JButton) event.getSource();

        if (pressedButton.getText() == "New Game") {
            game.play();
        }
        else if (isItUsersTurn) {
            selection = southPanel.getComponentZOrder(pressedButton) + 1;
            didUserMakeSelection = true;
        }
    }

    public int getSelection() {
        if (!isItUsersTurn) {
            isItUsersTurn = true;
        }

        if (didUserMakeSelection) {
            isItUsersTurn = false;
            didUserMakeSelection = false;
            return selection;
        }
        else {
            return 0;
        }
    }

}

---

The problem results from using the while loop

while (selection == 0) {
    selection = userInterface.getSelection();
}

in the play() method of Game.java.

If lines 12 and 14 are commented out,

//while (selection == 0) {
    selection = userInterface.getSelection();
//}

the program no longer freezes.

I think the problem is related to concurrency. However, I would like to gain a precise understanding of why the while loop causes the program to freeze.

Answer 1

Thank you fellow programmers. I found the answers very useful.

(1) Why does the program freeze?

When the program first starts, game.play() gets executed by the main thread, which is the thread that executes main. However, when the "New Game" button is pressed, game.play() gets executed by the event dispatch thread (instead of the main thread), which is the thread responsible for executing event-handling code and updating the user interface. The while loop (in play()) only terminates if selection == 0 evaluates to false. The only way selection == 0 evaluates to false is if didUserMakeSelection becomes true. The only way didUserMakeSelection becomes true is if the user presses one of the numbered buttons. However, the user cannot press any numbered button, nor the "New Game" button, nor exit the program. The "New Game" button doesn't even pop back out, because the event dispatch thread (which would otherwise repaint the screen) is too busy executing the while loop (which is effectively inifinte for the above reasons).

(2) How can the program be rewritten to fix the problem?

Since the problem is caused by the execution of game.play() in the event dispatch thread, the direct answer is to execute game.play() in another thread. This can be accomplished by replacing

if (pressedButton.getText() == "New Game") {
    game.play();
}

with

if (pressedButton.getText() == "New Game") {
    Thread thread = new Thread() {
        public void run() {
            game.play();
        }
    };
    thread.start();
}

However, this results in a new (albeit more tolerable) problem: Each time the "New Game" button is pressed, a new thread is created. Since the program is very simple, it's not a big deal; such a thread becomes inactive (i.e. a game finishes) as soon as the user presses a numbered button. However, suppose it took longer to finish a game. Suppose, while a game is in progress, the user decides to start a new one. Each time the user starts a new game (before finishing one), the number of active threads increments. This is undesirable, because each active thread consumes resources.

The new problem can be fixed by:

(1) adding import statements for Executors, ExecutorService, and Future, in Game.java

import java.util.concurrent.Executors;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;

(2) adding a single-thread executor as a field under Game

private ExecutorService gameExecutor = Executors.newSingleThreadExecutor();

(3) adding a Future, representing the last task submitted to the single-thread executor, as a field under Game

private Future<?> gameTask;

(4) adding a method under Game

public void startNewGame() {
    if (gameTask != null) gameTask.cancel(true);
    gameTask = gameExecutor.submit(new Runnable() {
        public void run() {
            play();
        }
    });
}

(5) replacing

if (pressedButton.getText() == "New Game") {
    Thread thread = new Thread() {
        public void run() {
            game.play();
        }
    };
    thread.start();
}

with

if (pressedButton.getText() == "New Game") {
    game.startNewGame();
}

and finally,

(6) replacing

public void play() {
    int selection = 0;

    while (selection == 0) {
        selection = userInterface.getSelection();
    }

    System.out.println(selection);
}

with

public void play() {
    int selection = 0;

    while (selection == 0) {
        selection = userInterface.getSelection();
        if (Thread.currentThread().isInterrupted()) {
            return;
        }
    }

    System.out.println(selection);
}

To determine where to put the if (Thread.currentThread().isInterrupted()) check, look at where the method lags. In this case, it is where the user has to make a selection.

There is another problem. The main thread could still be active. To fix this, you can replace

public static void main(String[] args) {
    Game game = new Game();
    game.play();
}

with

public static void main(String[] args) {
    Game game = new Game();
    game.startNewGame();
}

The code below applies the above modifications (in addition to a checkThreads() method):

import java.awt.BorderLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.concurrent.Executors;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;

public class Game {
    private GraphicalUserInterface userInterface;
    private ExecutorService gameExecutor = Executors.newSingleThreadExecutor();
    private Future<?> gameTask;

    public Game() {
        userInterface = new GraphicalUserInterface(this);
    }

    public static void main(String[] args) {
        checkThreads();
        Game game = new Game();
        checkThreads();
        game.startNewGame();
        checkThreads();
    }

    public static void checkThreads() {
        ThreadGroup mainThreadGroup = Thread.currentThread().getThreadGroup();
        ThreadGroup systemThreadGroup = mainThreadGroup.getParent();

        System.out.println("\n" + Thread.currentThread());
        systemThreadGroup.list();
    }

    public void play() {
        int selection = 0;

        while (selection == 0) {
            selection = userInterface.getSelection();
            if (Thread.currentThread().isInterrupted()) {
                return;
            }
        }

        System.out.println(selection);
    }

    public void startNewGame() {
        if (gameTask != null) gameTask.cancel(true);
        gameTask = gameExecutor.submit(new Runnable() {
            public void run() {
                play();
            }
        });
    }
}

class GraphicalUserInterface extends JFrame implements ActionListener {
    private Game game;
    private JButton newGameButton = new JButton("New Game");
    private JButton[] numberedButtons = new JButton[3];
    private JPanel southPanel = new JPanel();
    private int selection;
    private boolean isItUsersTurn = false;
    private boolean didUserMakeSelection = false;

    public GraphicalUserInterface(Game game) {
        this.game = game;

        newGameButton.addActionListener(this);

        for (int i = 0; i < 3; i++) {
            numberedButtons[i] = new JButton((new Integer(i+1)).toString());
            numberedButtons[i].addActionListener(this);
            southPanel.add(numberedButtons[i]);
        }

        getContentPane().add(newGameButton, BorderLayout.NORTH);
        getContentPane().add(southPanel, BorderLayout.SOUTH);

        pack();
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setLocationRelativeTo(null);
        setVisible(true);
    }

    public void actionPerformed(ActionEvent event) {
        JButton pressedButton = (JButton) event.getSource();

        if (pressedButton.getText() == "New Game") {
            game.startNewGame();
            Game.checkThreads();
        }
        else if (isItUsersTurn) {
            selection = southPanel.getComponentZOrder(pressedButton) + 1;
            didUserMakeSelection = true;
        }
    }

    public int getSelection() {
        if (!isItUsersTurn) {
            isItUsersTurn = true;
        }

        if (didUserMakeSelection) {
            isItUsersTurn = false;
            didUserMakeSelection = false;
            return selection;
        }
        else {
            return 0;
        }
    }
}

References

The Java Tutorials: Lesson: Concurrency
The Java Tutorials: Lesson: Concurrency in Swing
The Java Virtual Machine Specification, Java SE 7 Edition
The Java Virtual Machine Specification, Second Edition
Eckel, Bruce. Thinking in Java, 4th Edition. "Concurrency & Swing: Long-running tasks", p. 988.
How do I cancel a running task and replace it with a new one, on the same thread?

Answer 2

Strangely enough this problem does not have to do with concurrency, although your program is fraught with issues in that respect as well:

• main() is launched in the main application thread

• Once setVisible() is called in a Swing component, a new thread is created to handle the user interface

• Once a user presses the New Game button, the UI thread (not the main thread) calls, via the ActionEvent listener the Game.play() method, which goes into an infinite loop: the UI thread is constantly polling its own fields via the getSelection() method, without ever being given the chance to continue working on the UI and any new input events from the user.

  Essentially, you are polling a set of fields from the same thread that is supposed to change them - a guaranteed infinite loop that keeps the Swing event loop from getting new events or updating the display.

You need to re-design your application:

• It seems to me that the return value of getSelection() can only change after some user action. In that case, there is really no need to poll it - checking once in the UI thread should be enough.

• For very simple operations, such as a simple game that only updates the display after the user does something, it may be enough to perform all calculations in the event listeners, without any responsiveness issues.

• For more complex cases, e.g. if you need the UI to update without user intervention, such as a progress bar that fills-up as a file is downloaded, you need to do your actual work in separate threads and use synchronization to coordinate the UI updates.