CODEWITHSUNDEEP
java
Kcits970
Kcits970

Reputation: 642

Java unable to break out of while loop

I am trying to build a timer in java.

Expected Output:

When you run the program, there's a window with a big "0" in it.

Once the user presses any key from the keyboard, the number increases by 1 every second.

Here's what I have.

    import javax.swing.*;
    import javax.swing.SwingConstants;
    import java.awt.BorderLayout;
    import java.awt.Font;
    import java.awt.event.KeyListener;
    import java.awt.event.KeyEvent;

    public class TimerTest implements KeyListener {
        static final int SCREEN_WIDTH = 960;
        static final int SCREEN_HEIGHT = 540;
        static boolean timerStarted;
        static int keyPressedNum; //amount of times the user pressed any key.

        static JLabel label;
        static int currentTime;

    public static void main(String[] args) {

        JFrame frame = new JFrame("TimerTest");
        JPanel panel = new JPanel();

        label = new JLabel();
        label.setFont(new Font("Arial", Font.PLAIN, 256));
        label.setText("0");
        label.setHorizontalAlignment(SwingConstants.CENTER);

        panel.setLayout(new BorderLayout());
        panel.add(label, BorderLayout.CENTER);

        frame.addKeyListener(new TimerTest());

        frame.getContentPane().add(panel);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setSize(SCREEN_WIDTH, SCREEN_HEIGHT);
        frame.setResizable(false);
        frame.setVisible(true);

        while (true) {
            while (TimerTest.timerStarted == false) {
                if (TimerTest.timerStarted == true) {break;}
            }

            try {Thread.sleep(1000);}
            catch (InterruptedException ex) {ex.printStackTrace();}

            currentTime++;
            label.setText(String.valueOf(currentTime));
        }
    }

    public void keyPressed(KeyEvent e) {
        System.out.println("Keypress indicated.");
        TimerTest.timerStarted = true;
    }

    public void keyReleased(KeyEvent e) {}
    public void keyTyped(KeyEvent e) {}
}

When I press any key, the program sets timerStarted as true.

Because timerStarted is true, I was expecting this part:

    while (TimerTest.timerStarted == false) {
            if (TimerTest.timerStarted == true) {break;}
    }

to break out of the loop.

Instead when I run the program and press any key, the command line just prints: Keypress indicated., and it doesn't break out of that loop.

Now here's the weird part:

I tried adding some code to the while block, like this.

  while (TimerTest.timerStarted == false) {
        System.out.println("currently stuck here...");
        //(I simply added a println code)
        if (TimerTest.timerStarted == true) {break;}
  }

Surprisingly when I do this, the program works just like how I wanted. It breaks out of that loop and the timer runs.

This confuses me. Adding a System.out.println(); fixes the issue...?

Why does not having System.out.println() makes me unable to break out of the loop?

Any explanations would be appreciated =)

Upvotes: 0

Views: 118

Answers (2)

Stephen C
Stephen C

Reputation: 718718

The problem is that your code is not thread-safe. Specifically, you are not doing the things that are needed to ensure that reads of timerStarted see the most recent write by the other thread.

There are number of possible solutions, including:

  • declare timerStarted as volatile, or
  • read and write timerStarted in a synchronized block, or
  • use an AtomicBoolean.

These ensure that there is a happens-before relation between a write of the flag by one thread and subsequent read by a second thread. That in turn guarantees that the read sees the value that was written.

"Why does not having System.out.println() makes me unable to break out of the loop?"

Because there is no happens-before relationship; see above.

A more interesting question is:

"Why does it work when you add the println?"

The System.out.println() does some synchronization under the hood. (The stream object is thread-safe, and synchronizes to ensure that its data structures don't get messed up.) This is apparently sufficient to cause writes to timerStarted to be flushed to main memory.

However, this is not guaranteed. The javadocs do not specify the synchronization behavior for the println call. Furthermore, it is not clear that this is sufficient to give a (serendipitous!) happens before between the write and read of timerStarted.

References:

Upvotes: 1

Atef Hares
Atef Hares

Reputation: 4881

Your issue is because you are using a non thread-safe boolean variable timerStarted

moreover the while(true) loop is running from main thread but the modification you do on timerStarted is done from another thread this gives unexpected behavior completely depends on the machine that runs the program.

This situation is making a race-condition between changing value of timerStarted and checking for that change

How did I detected this:

Simply I printed the thread id while checking for value changes and while modifying the value

The solution

check these code changes

static AtomicBoolean timerStarted = new AtomicBoolean(false);

and 

   while (Scratch.timerStarted.get() == false) {
        System.out.println("Thread.ID: " + Thread.currentThread().getId() + " ----> Scratch.timerStarted: " + Scratch.timerStarted);
        if (Scratch.timerStarted.get() == true) {
            break;
        }
    }

and 

 public void keyPressed(KeyEvent e) {
        System.out.println("Keypress indicated.");
        Scratch.timerStarted.set(true);
        System.out.println("Thread.ID: " + Thread.currentThread().getId() + " ----> Scratch.timerStarted: " + Scratch.timerStarted);
    }

here I used AtomicBoolean instead of boolean which is thread safe.

What you need to read about:

  1. Multi-threading in java (check this: https://www.javatpoint.com/multithreading-in-java)
  2. AtmoicBoolean vs boolean (check this: When do I need to use AtomicBoolean in Java?)

Upvotes: 1

Related Questions