Properly manage threads in JavaFX

Question

I'm trying to call an external command and print its output in a TextArea while it is produced. I read the doc about concurrency in JavaFX and I believe I did what I had to do to make it work.

I'm using the Task class to run my job :

public synchronized void run(ProcessBuilder processBuilder) throws IOException, InterruptedException {
    ExternalCommandRunner self = this;
    Thread taskThread = new Thread(new Task<Void>() {
        @Override
        public Void call() throws Exception {
            setActive();
            try {
                runningProcess = processBuilder.start();

                StreamPrinter  inputStream    = new StreamPrinter(runningProcess.getInputStream(), self::handleLog);
                StreamPrinter  errorStream    = new StreamPrinter(runningProcess.getErrorStream(), self::handleLog);
                outputTextArea.clear();

                new Thread(inputStream).start();
                new Thread(errorStream).start();
                runningProcess.waitFor();
                return null;
            } finally {
                stop.fire();
                setInactive();
            }
        }
    });
    taskThread.setDaemon(true);
    taskThread.start();
    taskThread.join();
}

private void handleLog  (String line) { Platform.runLater(() -> outputTextArea.appendText(line + "\n")); }
private void setActive  ()            { setState(STOP_ACTIVE_ICON  , false)                            ; }
private void setInactive()            { setState(STOP_INACTIVE_ICON, true)                             ; }
private void setState   (String iconPath, boolean disableButton) { 
    stop.setGraphic(imageViewFromResource(iconPath, Resources.class));
    stop.setDisable(disableButton); 
}

However, I'm getting the following exception :

Exception in thread "Thread-5" java.lang.IllegalStateException: Not on FX application thread; currentThread = Thread-5
    at com.sun.javafx.tk.Toolkit.checkFxUserThread(Unknown Source)
    at com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(Unknown Source)
    at javafx.scene.Parent$2.onProposedChange(Unknown Source)
    at com.sun.javafx.collections.VetoableListDecorator.setAll(Unknown Source)
    at com.sun.javafx.collections.VetoableListDecorator.setAll(Unknown Source)
    at com.sun.javafx.scene.control.skin.LabeledSkinBase.updateChildren(Unknown Source)
    at com.sun.javafx.scene.control.skin.LabeledSkinBase.handleControlPropertyChanged(Unknown Source)
    at com.sun.javafx.scene.control.skin.ButtonSkin.handleControlPropertyChanged(Unknown Source)
    at com.sun.javafx.scene.control.skin.BehaviorSkinBase.lambda$registerChangeListener$61(Unknown Source)
    at com.sun.javafx.scene.control.MultiplePropertyChangeListenerHandler$1.changed(Unknown Source)
    at javafx.beans.value.WeakChangeListener.changed(Unknown Source)
    at com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(Unknown Source)
    at com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(Unknown Source)
    at javafx.beans.property.ObjectPropertyBase.fireValueChangedEvent(Unknown Source)
    at javafx.beans.property.ObjectPropertyBase.markInvalid(Unknown Source)
    at javafx.beans.property.ObjectPropertyBase.set(Unknown Source)
    at javafx.css.StyleableObjectProperty.set(Unknown Source)
    at javafx.beans.property.ObjectProperty.setValue(Unknown Source)
    at javafx.scene.control.Labeled.setGraphic(Unknown Source)
    at com.dici.javafx.components.ExternalCommandRunner.setState(ExternalCommandRunner.java:66)
    at com.dici.javafx.components.ExternalCommandRunner.setActive(ExternalCommandRunner.java:63)
    at com.dici.javafx.components.ExternalCommandRunner.access$000(ExternalCommandRunner.java:19)
    at com.dici.javafx.components.ExternalCommandRunner$1.call(ExternalCommandRunner.java:39)
    at com.dici.javafx.components.ExternalCommandRunner$1.call(ExternalCommandRunner.java:36)
    at javafx.concurrent.Task$TaskCallable.call(Unknown Source)
    at java.util.concurrent.FutureTask.run(Unknown Source)
    at java.lang.Thread.run(Unknown Source)

stop is simply a Button. What am I not doing right ? From the doc, using a Taskthis way should be enough...

Is that because I am blocking the FX application thread with `taskThread.join()` ? I needed this to prevent other threads to interfere with the `TextArea` while the command is running. I'm going to try another synchronization method — Dici, Sep 28 '15 at 12:43
The error says you are updating a UI control (specifically, setting the graphic on a label) from a background thread. All updates to the UI must occur on the FX Application Thread. You also must not block the FX Application Thread, which you appear to be doing via your call to `join()`. So I think there are many errors here. Maybe see http://stackoverflow.com/questions/30249493/using-threads-to-make-database-requests which might help a bit - though your use case looks a bit different. It would probably help if you described what you are trying to achieve and created a [MCVE]. — James_D, Sep 28 '15 at 12:54
I'm now using a `Semaphore` to block the access to the method without blocking the FX main thread, but still getting the error. According to the doc of `Task`, using `new Thread(new Task<...>() { ... }).start()` should just work, so I'm confused. Note that I'm now able to update the `TextArea` correctly, but not the icon in the button — Dici, Sep 28 '15 at 12:58
It's pretty simple: on line 66 you are calling `setGraphic(...)` on a `Labeled` (presumably your `Button`) from the background thread. As is [widely documented](http://docs.oracle.com/javase/8/javafx/api/javafx/application/Application.html) you can only do that from the FX Application Thread. So you must either wrap that code in `Platform.runLater(...)` or (better) structure the code so that call is naturally invoked from the FX Application Thread. — James_D, Sep 28 '15 at 13:04
It's also not really clear why you need any synchronization at all, since any access to the `TextArea` is necessarily performed on the FX Application Thread. — James_D, Sep 28 '15 at 13:05
Isn't the `Task` class supposed to make sure the calls on UI components occur in the right thread ? That's what I read from the doc http://docs.oracle.com/javase/8/javafx/interoperability-tutorial/concurrency.htm. `Platform.runLater` is said to be used for light tasks, `Task` for heavy ones. The synchronization is needed to prevent people from running several external commands at the same time as the component I'm writing must act as a console. I don't want several processes to right to it at the same time. This is no longer an issue because I rewrote it using a semaphore. — Dici, Sep 28 '15 at 13:09
Uh, no. It's not magic. It simply provides API to make it easy for you to write code that is invoked on the FX Application Thread. But if you put code in a method, then that code is executed in the thread in which that method is invoked. You probably want to call `setActive()` outside of the task, right before you call `taskThread.start()`, and call `stop.fire(); setInactive();` either in an `onSucceeded` handler or in a listener to the task's `stateProperty()`. But I still don't fully understand what you're trying to achieve. — James_D, Sep 28 '15 at 13:14
And it's ***impossible*** for several processes to write to the text area at the same time, because those processes ***must*** schedule writes to the text area on the FX Application Thread. I don't really understand why you don't just return the result of your external call from your task, and update the text area from the `onSucceeded` handler. — James_D, Sep 28 '15 at 13:15
I re-read the doc and, the `call` method must indeed not manipulate the UI. I refactored the code to execute the UI updates outside of the `Task`. What I meant by *multiple processes writing at the same time* is that I am displaying the output of the command, which is continuously produced until termination. Since I am submitting this output line by line to the UI using `Platform.runLater`, if I let several threads access my `run` methods, they will all be able to write in the text area, which means I will be displaying the ouput of different commands at the same time. — Dici, Sep 28 '15 at 13:23

score 1 · Answer 1 · edited Sep 29 '15 at 10:32

If I get it right, the stacktrace simply says that there is another line in the code where some UI control is manipulated from outside the FX application thread - and that must not happen.

The first line of the application's code down the trace (not JDK code) is: at com.dici.javafx.components.ExternalCommandRunner.setState(ExternalCommandRunner.java:66)

This seems to be foowloing lines in your example:

stop.setGraphic(imageViewFromResource(iconPath, Resources.class));
stop.setDisable(disableButton);

If stop is just a button and the method setState() manipulates it and is called from your own custom thread, then exactly that happens what the framework complains.

Try putting anything that manipulates UI controls on the FX Application Thread, exactly as you did with Platform.runLater(() -> { ... }) in the handleLog method.

That was one of the problems indeed, coming from a misunderstanding of the `Task` doc. I am not using `Platform.runLater` because it is expected to be used for short tasks whereas the one I am submitting can last several seconds. The answer lies in @James_D comments, if someone compiles them to make an answer I will accept it. Otherwise, I will do it myself after a while — Dici, Sep 29 '15 at 10:03

score 0 · Accepted Answer · answered Sep 29 '15 at 18:19

Stupid me. I wanted to guarantee that nobody would be able to run a command (and print the output on the text area) while another command was already running, hence the synchronized and Thread.join. However, this was wrong. I also misunderstood the documentation on the Task class.

Here are the two mistakes I have made and how to fix them :

Thread.join blocks the FX application thread until the task is over, so nothing is ever displayed in the text area. I replaced this synchronization using a Semaphore. The semaphore prevents a second thread to enter the run method and is released when the background thread terminates, using the Task listener methods. The FX application thread exits the method immediately.
Task.call must actually not interact with any JavaFX component, according to the doc. Only the listener methods (cancelled, succeeded, failed) are allowed to do so. I thus extracted the calls to setActive and setInactive to outside of the call method.

The code demonstrating these changes is the following :

private final Semaphore runMutex = new Semaphore(1);

public void run(ProcessBuilder processBuilder) throws IOException, InterruptedException {
    runMutex.acquire();
    setActive();
    ExternalCommandRunner self = this;
    Thread taskThread = new Thread(new Task<Void>() {
        @Override
        public Void call() throws Exception {
            runningProcess = processBuilder.start();

            StreamPrinter  inputStream = new StreamPrinter(runningProcess.getInputStream(), self::handleLog);
            StreamPrinter  errorStream = new StreamPrinter(runningProcess.getErrorStream(), self::handleLog);
            outputTextArea.clear();

            new Thread(inputStream).start();
            new Thread(errorStream).start();
            runningProcess.waitFor();
            return null;
        }

        @Override protected void cancelled() { super.cancelled(); terminate(); }
        @Override protected void failed   () { super.failed   (); terminate(); }
        @Override protected void succeeded() { super.succeeded(); terminate(); }

        private void terminate() {
            stop.fire();
            setInactive();
            runMutex.release();
        }
    });
    taskThread.setDaemon(true);
    taskThread.start();
}

Properly manage threads in JavaFX

2 Answers2