Timer in Javafx Creating FX Thread null pointers and out of bounds exceptions

Question

I was trying to make a simple timer in Javafx with a GUI and Timer class. When the start button is pressed, it is supposed to count down from the inputed time and stop when it reaches 0.

I have the remaining time in milliseconds updating in a TextField on the GUI but it only runs for a random number of milliseconds usually between 100-200 and then it freezes up and throws a VERY large number of exceptions.

I tried to pinpoint where the error was coming from, and found there was also a concurrent modification exception as well.

Here is my Timer class:

import java.sql.Time;

/**
 * Created by Josh on 19/8/2015.
 */
public class Timer {

    private long endTimeMillis;

    public Timer(long hours, long minutes, long seconds){

        long currentTime = System.currentTimeMillis();

        endTimeMillis = ((seconds*1000 + minutes*1000*60 + hours*1000*60*60) + currentTime);

    }

    public boolean isFinished(){

        long currentTime = System.currentTimeMillis();

        if(endTimeMillis - currentTime <= 0){
            return true;
        }else{
            return false;
        }

    }

    public long getRemainingTimeMillis(){

        long currentTime = System.currentTimeMillis();

        return endTimeMillis - currentTime;

    }

}

and this is the GUI

 import javafx.application.Application;
import javafx.application.Platform;
import javafx.concurrent.Task;
import javafx.event.ActionEvent;
import javafx.event.EventHandler;
import javafx.geometry.Pos;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.FlowPane;
import javafx.stage.Stage;
import javafx.stage.WindowEvent;

/**
 * Created by Josh on 19/8/2015.
 */
public class GUI extends Application {

    private BorderPane root;

    Label hoursLabel, minutesLabel, secondsLabel;
    TextField hoursTF, minutesTF, secondsTF;

    Button startButton;

    Label remainingTimeLabel;
    TextField remainingTimeTF;

    long remainingTime;

    @Override
    public void start(Stage primaryStage) throws Exception {

        root = new BorderPane();
        primaryStage.setScene(new Scene(root, 1300, 125));
        primaryStage.show();
        primaryStage.setResizable(true);
        primaryStage.setTitle("Timer");
        primaryStage.setOnCloseRequest(new EventHandler<WindowEvent>() {
            @Override
            public void handle(WindowEvent event) {
                event.consume();
                System.exit(0);
            }
        });

        FlowPane center = new FlowPane();
        hoursLabel = new Label("     Hours: ");
        hoursTF = new TextField();
        minutesLabel = new Label("     Minutes: ");
        minutesTF = new TextField();
        secondsLabel = new Label("     Seconds: ");
        secondsTF = new TextField();
        center.getChildren().addAll(hoursLabel, hoursTF, minutesLabel, minutesTF, secondsLabel, secondsTF);
        root.setCenter(center);

        FlowPane bottom = new FlowPane();
        bottom.setAlignment(Pos.CENTER);
        startButton = new Button("Start");
        startButton.setOnAction(new EventHandler<ActionEvent>() {
            @Override
            public void handle(ActionEvent event) {
                start();
            }
        });
        remainingTimeLabel = new Label("     Time Remaining: ");
        remainingTimeTF = new TextField();
        bottom.getChildren().addAll(startButton, remainingTimeLabel, remainingTimeTF);
        root.setBottom(bottom);


    }



    public void start(){

        Task timer = new Task<Void>() {
            @Override
            protected Void call() throws Exception {
                long hours = 0, minutes = 0, seconds = 0;

                try{
                    if(hoursTF.getText() == null || hoursTF.getText().equals("")){
                        hours = 0;
                    }else{
                        hours = Long.parseLong(hoursTF.getText());
                    }
                    if(minutesTF.getText() == null || minutesTF.getText().equals("")){
                        minutes = 0;
                    }else{
                        minutes = Long.parseLong(minutesTF.getText());
                    }
                    if(secondsTF.getText() == null || secondsTF.getText().equals("")){
                        seconds = 0;
                    }else{
                        seconds = Long.parseLong(secondsTF.getText());
                    }
                }catch(NumberFormatException e){
                    System.out.println("Error");

                }

                Timer timer = new Timer(hours, minutes, seconds);

                while(!timer.isFinished()){
                    remainingTimeTF.setText(Long.toString(timer.getRemainingTimeMillis()));
                }


                return null;
            }
        };
        new Thread(timer).start();

    }

    public static void main(String[] args) {
        launch(args);
    }
}

any help would be appreciated!

New stack trace:

    x.beans.property.StringPropertyBase.set(StringPropertyBase.java:49)
    at javafx.beans.property.StringProperty.setValue(StringProperty.java:65)
    at javafx.scene.control.Labeled.setText(Labeled.java:145)
    at GUI$3.call(GUI.java:109)
    at GUI$3.call(GUI.java:80)
    at javafx.concurrent.Task$TaskCallable.call(Task.java:1423)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.lang.Thread.run(Thread.java:745)
Exception in thread "Thread-4" java.lang.IllegalStateException: Not on FX application thread; currentThread = Thread-4
    at com.sun.javafx.tk.Toolkit.checkFxUserThread(Toolkit.java:204)
    at com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(QuantumToolkit.java:438)
    at javafx.scene.Parent$2.onProposedChange(Parent.java:364)
    at com.sun.javafx.collections.VetoableListDecorator.setAll(VetoableListDecorator.java:113)
    at com.sun.javafx.collections.VetoableListDecorator.setAll(VetoableListDecorator.java:108)
    at com.sun.javafx.scene.control.skin.LabeledSkinBase.updateChildren(LabeledSkinBase.java:575)
    at com.sun.javafx.scene.control.skin.LabeledSkinBase.handleControlPropertyChanged(LabeledSkinBase.java:204)
    at com.sun.javafx.scene.control.skin.LabelSkin.handleControlPropertyChanged(LabelSkin.java:49)
    at com.sun.javafx.scene.control.skin.BehaviorSkinBase.lambda$registerChangeListener$61(BehaviorSkinBase.java:197)
    at com.sun.javafx.scene.control.skin.BehaviorSkinBase$$Lambda$102/1442917786.call(Unknown Source)
    at com.sun.javafx.scene.control.MultiplePropertyChangeListenerHandler$1.changed(MultiplePropertyChangeListenerHandler.java:55)
    at javafx.beans.value.WeakChangeListener.changed(WeakChangeListener.java:89)
    at com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:182)
    at com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:81)
    at javafx.beans.property.StringPropertyBase.fireValueChangedEvent(StringPropertyBase.java:103)
    at javafx.beans.property.StringPropertyBase.markInvalid(StringPropertyBase.java:110)
    at javafx.beans.property.StringPropertyBase.set(StringPropertyBase.java:144)
    at javafx.beans.property.StringPropertyBase.set(StringPropertyBase.java:49)
    at javafx.beans.property.StringProperty.setValue(StringProperty.java:65)
    at javafx.scene.control.Labeled.setText(Labeled.java:145)
    at GUI$3.call(GUI.java:109)
    at GUI$3.call(GUI.java:80)
    at javafx.concurrent.Task$TaskCallable.call(Task.java:1423)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.lang.Thread.run(Thread.java:745)

Answer 1

You are violating the rule about not accessing the state of a node in a live scene graph on a background thread. Most JavaFX controls will actually check for this and throw an IllegalStateException when you do this: apparently TextInputControl.setText(...) doesn't (presumably for reasons of performance).

The simplest fix is to update the message property of the task:

            while(!timer.isFinished()){
              updateMessage(Long.toString(timer.getRemainingTimeMillis()));
              // remainingTimeTF.setText(Long.toString(timer.getRemainingTimeMillis()));
            }

(this causes an update to the message property to occur on the FX Application Thread), and then bind the text field's text property to it:

    remainingTimeTF.textProperty().bind(timer.messageProperty());
    new Thread(timer).start();

However, there are a lot of things about your code that violate good practice. Fo example, you should really compute the total time outside of the task (i.e. on the FX Application Thread) and store it in a final variable; and the busy while loop is pretty horrible (though updateMessage(...) will fix most of the issues associated with that for you).

I recommend you read up on multithreading in JavaFX. Start with the Task API docs. Maybe this question will help: Using threads to make database requests

Answer 2

First of all, if you really need to update that TextField in the way you are doing it I would recommend adding a Thread.sleep(100); (10 updates per second) in your while, or even Thread.sleep(1000); (1 update per second) since it's really CPU intensive to update that text field on every CPU cycle and usually you don't need to..

Secondly (and probably the reason of your exceptions) the call remainingTimeTF.setText(); MUST happen IN the FX Thread, try this code instead:

Platform.runLater(new Runnable() {
    @Override public void run() {
        remainingTimeTF.setText(Long.toString(timer.getRemainingTimeMillis()));
    }
});

Right now you are setting the text outside the FX Thead which might lead to several exceptions since update requests are happening in a worker thread and the actual UI modifications are happening in the FX thread. You need to make sure that every time you need to modify the UI it happens in the correct thread, in this case the FX Thread with the Platform.runLater() call.

Also since you are already using the Task class why not bind the text property and use the updateMessage() or updateTitle() methods instead? Take a look at this article, it has good examples for the correct usage of Task and updating the UI from worker threads.

Hope this helps!