5

My application contains a ListView that kicks off a background task every time an item is selected. The background task then updates information on the UI when it completes successfully.

However, when the user quickly clicks one item after another, all these tasks continue and the last task to complete "wins" and updates the UI, regardless of which item was selected last.

What I need is to somehow ensure this task only has one instance of it running at any given time, so cancel all prior tasks before starting the new one.

Here is an MCVE that demonstrates the issue:

import javafx.application.Application;
import javafx.concurrent.Task;
import javafx.geometry.Insets;
import javafx.geometry.Pos;
import javafx.scene.Scene;
import javafx.scene.control.Label;
import javafx.scene.control.ListView;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class taskRace  extends Application {

    private final ListView<String> listView = new ListView<>();
    private final Label label = new Label("Nothing selected");
    private String labelValue;

    public static void main(String[] args) {

        launch(args);
    }

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

        // Simple UI
        VBox root = new VBox(5);
        root.setAlignment(Pos.CENTER);
        root.setPadding(new Insets(10));
        root.getChildren().addAll(listView, label);

        // Populate the ListView
        listView.getItems().addAll(
                "One", "Two", "Three", "Four", "Five"
        );

        // Add listener to the ListView to start the task whenever an item is selected
        listView.getSelectionModel().selectedItemProperty().addListener((observableValue, oldValue, newValue) -> {

            if (newValue != null) {

                // Create the background task
                Task task = new Task() {
                    @Override
                    protected Object call() throws Exception {

                        String selectedItem = listView.getSelectionModel().getSelectedItem();

                        // Do long-running task (takes random time)
                        long waitTime = (long)(Math.random() * 15000);
                        System.out.println("Waiting " + waitTime);
                        Thread.sleep(waitTime);
                        labelValue = "You have selected item: " + selectedItem ;
                        return null;
                    }
                };

                // Update the label when the task is completed
                task.setOnSucceeded(event ->{
                    label.setText(labelValue);
                });

                new Thread(task).start();
            }

        });

        stage.setScene(new Scene(root));
        stage.show();

    }
}

When clicking on several items in random order, the outcome is unpredictable. What I need is for the label to be updated to show the results from the last Task that was executed.

Do I need to somehow schedule the tasks or add them to a service in order to cancel all prior tasks?

EDIT:

In my real world application, the user selects an item from the ListView and the background task reads a database ( a complex SELECT statement) to get all the information associated with that item. Those details are then displayed in the application.

The issue that is happening is when the user selects an item but changes their selection, the data returned displayed in the application could be for the first item selected, even though a completely different item is now chosen.

Any data returned from the first (ie: unwanted) selection can be discarded entirely.

Zephyr
  • 9,885
  • 4
  • 28
  • 63
  • Could I know what really the task does? I mean what business rule for the task? And then when you said cancel, what does it really mean? Suppose if a task is performing a REST call or perform a Stored Procedure ... we need to know what really cancelled task is. – Nghia Do Jul 15 '18 at 22:02
  • @NghiaDo - See my edit. Thank you. – Zephyr Jul 15 '18 at 22:06
  • So, should end-user wait until the task finish before allow user select a new item? – Nghia Do Jul 15 '18 at 22:10
  • No, I would like them to be able to move on and select a new item whenever they wish. – Zephyr Jul 15 '18 at 22:11
  • What is the max time of a task may take? – Nghia Do Jul 15 '18 at 22:30
  • It is dependent on too many factors and can vary greatly depending on the item selected. – Zephyr Jul 15 '18 at 22:53
  • I have created a tiny example to see if it fits for your case or not. Please see the answer. – Nghia Do Jul 16 '18 at 00:46

5 Answers5

4

Your requirements, as this answer mentions, seem like a perfect reason for using a Service. A Service allows you to run one Task at any given time in a reusable1 manner. When you cancel a Service, via Service.cancel(), it cancels the underlying Task. The Service also keeps track of its own Task for you so you don't need to keep them in a list somewhere.

Using your MVCE what you'd want to do is create a Service that wraps your Task. Every time the user selects a new item in the ListView you'd cancel the Service, update the necessary state, and then restart the Service. Then you'd use the Service.setOnSucceeded callback to set the result to the Label. This guarantees that only the last successful execution will be returned to you. Even if previously cancelled Tasks still return a result the Service will ignore them.

You also don't need to worry about external synchronization (at least in your MVCE). All the operations that deal with starting, cancelling, and observing the Service happen on the FX thread. The only bit of code (featured below) not executed on the FX thread will be inside Task.call() (well, and the immediately assigned fields when the class gets instantiated which I believe happens on the JavaFX-Launcher thread).

Here is a modified version of your MVCE using a Service:

import javafx.application.Application;
import javafx.concurrent.Service;
import javafx.concurrent.Task;
import javafx.geometry.Insets;
import javafx.geometry.Pos;
import javafx.scene.Scene;
import javafx.scene.control.Label;
import javafx.scene.control.ListView;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class Main extends Application {

    private final ListView<String> listView = new ListView<>();
    private final Label label = new Label("Nothing selected");

    private final QueryService service = new QueryService();

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

    @Override
    public void start(Stage stage) throws Exception {
        service.setOnSucceeded(wse -> {
            label.setText(service.getValue());
            service.reset();
        });
        service.setOnFailed(wse -> {
            // you could also show an Alert to the user here
            service.getException().printStackTrace();
            service.reset();
        });

        // Simple UI
        VBox root = new VBox(5);
        root.setAlignment(Pos.CENTER);
        root.setPadding(new Insets(10));
        root.getChildren().addAll(listView, label);

        // Populate the ListView
        listView.getItems().addAll(
                "One", "Two", "Three", "Four", "Five"
        );

        listView.getSelectionModel().selectedItemProperty().addListener((observableValue, oldValue, newValue) -> {
            if (service.isRunning()) {
                service.cancel();
                service.reset();
            }
            service.setSelected(newValue);
            service.start();
        });

        stage.setScene(new Scene(root));
        stage.show();

    }

    private static class QueryService extends Service<String> {

        // Field representing a JavaFX property
        private String selected;

        private void setSelected(String selected) {
            this.selected = selected;
        }

        @Override
        protected Task<String> createTask() {
            return new Task<>() {

                // Task state should be immutable/encapsulated
                private final String selectedCopy = selected;

                @Override
                protected String call() throws Exception {
                    try {
                        long waitTime = (long) (Math.random() * 15_000);
                        System.out.println("Waiting " + waitTime);
                        Thread.sleep(waitTime);
                        return "You have selected item: " + selectedCopy;
                    } catch (InterruptedException ex) {
                        System.out.println("Task interrupted!");
                        throw ex;
                    }
                }

            };
        }

        @Override
        protected void succeeded() {
            System.out.println("Service succeeded.");
        }

        @Override
        protected void cancelled() {
            System.out.println("Service cancelled.");
        }

    }
}

When you call Service.start() it creates a Task and executes it using the current Executor contained in its executor property. If the property contains null then it uses some unspecified, default Executor (using daemon threads).

Above, you see I call reset() after cancelling and in the onSucceeded and onFailed callbacks. This is because a Service can only be started when in the READY state. You can use restart() rather than start() if needed. It is basically equivalent to calling cancel()->reset()->start().

1The Task doesn't become resuable. Rather, the Service creates a new Task each time it's started.


When you cancel a Service it cancels the currently running Task, if there is one. Even though the Service, and therefore Task, have been cancelled does not mean that execution has actually stopped. In Java, cancelling background tasks requires cooperation with the developer of said task.

This cooperation takes the form of periodically checking if execution should stop. If using a normal Runnable or Callable this would require checking the interrupted status of the current Thread or using some boolean flag2. Since Task extends FutureTask you can also use the isCancelled() method inherited from the Future interface. If you can't use isCancelled() for some reason (called outside code, not using a Task, etc...) then you check for thread interruption using:

  • Thread.interrupted()
    • Static method
    • Can only check current Thread
    • Clears the interruption status of current thread
  • Thread.isInterrupted()
    • Instance method
    • Can check any Thread you have a reference to
    • Does not clear interruption status

You can get a reference to the current thread via Thread.currentThread().

Inside your background code you'd want to check at appropriate points if the current thread has been interrupted, the boolean flag has been set, or the Task has been cancelled. If it has then you'd perform any necessary clean up and stop execution (either by returning or throwing an exception).

Also, if your Thread is waiting on some interruptible operation, such as blocking IO, then it will throw an InterruptedException when interrupted. In your MVCE you use Thread.sleep which is interruptible; meaning when you call cancel this method will throw the mentioned exception.

When I say "clean up" above I mean any clean up necessary in the background since you're still on the background thread. If you need to clean up anything on the FX thread (such as updating the UI) then you can use the onCancelled property of Service.

In the code above you'll also see I use the protected methods succeeded() and cancelled(). Both Task and Service provide these methods (as well as others for the various Worker.States) and they will always be called on the FX thread. Read the documentation, however, as ScheduledService requires you to call the super implementations for some of these methods.

2If using a boolean flag make sure updates to it are visible to other threads. You can do this by making it volatile, synchronizing it, or using a java.util.concurrent.atomic.AtomicBoolean.

Slaw
  • 37,820
  • 8
  • 53
  • 80
  • Thank you. This actually worked perfectly! It did take a bit of work to adapt to a custom object (I created a new object to hold the individual data objects needed), but I like it. The other background tasks still execute, but since there are only 2 database queries, I can't cancel the first and the second is super fast anyway). – Zephyr Jul 20 '18 at 22:31
  • 1
    @Zephyr I'm glad I could help. I was actually just starting to answer your comments when you figured it out, but I'll give the highlights of what I was going to say anyway. (1) Remember the golden rule of JavaFX: _Never update the UI, directly or indirectly, from a thread other than the JavaFX Application Thread_, (2) It'd be preferable to have the input and output objects for the `Service` be immutable; but if they can't be then just make sure properly guard against multi-threading (maintain _happens-before_ relationships), (3) The `Task` should receive a copy of the input so state... (cont) – Slaw Jul 20 '18 at 22:44
  • ... doesn't change while the `Task` is executing (when the input is immutable just copy the reference), and (4) I recommend keeping UI code separate from the `Service` unless its already an inner class of a controller. The obvious exception for this being what you do in the `setOnXXX` callbacks. – Slaw Jul 20 '18 at 22:45
  • I am actually having the task create a new object and returning that. The input is just telling the task what query to run on the database and the new object is updated with that data. – Zephyr Jul 20 '18 at 22:47
  • Incidentally, are the `succeeded()` and `canceled()` methods within the `Service` necessary? Or is that just for cleanup, if needed? – Zephyr Jul 20 '18 at 22:49
  • 1
    @Zephyr I believe those `protected` methods are there more so you can do certain cleanup _inside_ the `Service`. For instance, `ScheduledService` uses some of them to update any properties needed then reschedule itself. Also, you could use the `failed()` method to always log a failure rather than having that behavior coded into `setOnFailed`. But no I don't believe they're necessary in your case. I mostly put them in my example to simply show they exist. – Slaw Jul 20 '18 at 22:57
0

Calling javafx.concurrent.Service.cancel() will cancel any currently running Task, making it throw an InterruptedException.

Cancels any currently running Task, if any. The state will be set to CANCELLED.
~ Service (JavaFX 8) - cancel ~

If additional cleanup is required one can provide Task.onCancelled() event handler.

Roshana Pitigala
  • 8,437
  • 8
  • 49
  • 80
Karol Dowbecki
  • 43,645
  • 9
  • 78
  • 111
  • Given the MCVE in the original question, where would one call the `cancel()` method? Does each task need to be added to a list and then iterated over, cancelling all tasks? – Zephyr Jul 15 '18 at 21:11
  • It will only throw interrupted exception if the process is IO bound. There's no guarantee that in the real world that the worker task is in sleep state. – flakes Jul 16 '18 at 00:34
0

The demo class

package com.changehealthcare.pid;

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;

public class ThreadFun {
    public static ExecutorService threadExecutor = Executors.newFixedThreadPool(1);
    public static ServiceThread serviceThreads = new ServiceThread();
    public static Future futureService;

    public static void main(String[] args) throws Exception {
        BufferedReader in = new BufferedReader(new InputStreamReader(System.in));
        String line = "";

           while (line.equalsIgnoreCase("quit") == false) {
               line = in.readLine();
               //do something
               if (futureService != null) {
                   System.out.println("try to cancel");
                   futureService.cancel(true);
               }
               futureService = threadExecutor.submit(serviceThreads);
           }

           in.close();

    }
}

The ServiceThread class

package com.changehealthcare.pid;

public class ServiceThread implements Runnable {

    @Override
    public void run() {
        System.out.println("starting the service");

        boolean isResult = this.doStuff();

        if (isResult) {
            this.displayStuff();
        }
        System.out.println("done");
    }

    private boolean doStuff() {
        try {
            for (int i=0; i<=10; i++) {
                System.out.println(i);
                Thread.sleep(1000);
            }

            return true;
        }
        catch (InterruptedException e) {
            System.out.println("cancelled");
            return false;
        }
    }

    private void displayStuff() {
        System.out.println("display done");
    }
}

Explaination The demo application acts as a listener for System.in (keyboard). If any character pressed (with Enter), it will cancel any current thread if existing and submit a new one.

The Service Thread demo has a run() method which is calling doStuff() and displayStuff(). You can inject UI part for displayStuff. In the doStuff(), it is catching InterupptedException, if that exception happens, we don't perform displayStuff().

Please check and let see if it fits for your case. We can work out more.

Nghia Do
  • 2,588
  • 2
  • 17
  • 31
  • In this case OP wants to eventually get the result of the thread back into the parent / draw thread. It would be more relevant to submit a `Callable` instead of a `Runnable`. That way you end up with a `Future` instead of a `Future>` – flakes Jul 16 '18 at 00:55
  • I am confused, though. Where does this allow me to cancel all previous tasks when a new one is started? – Zephyr Jul 16 '18 at 03:26
  • There is always max one previous task – Nghia Do Jul 16 '18 at 11:27
0

The simplest code that does what you need is this:

class SingleTaskRunner {

    private Task<?> lastTask = null; 

    void runTask(Task<?> task) {
        registerTask(task);
        new Thread(task).start();
    }

    private synchronized void registerTask(Task<?> task) {
        if (lastTask != null) {
            lastTask.cancel(true);
        }
        lastTask = task;
    }
}

Note that it's quite similar to Nghia Do's answer, only with synchronization and without all this extra demo code.

You commented that you're confused how the mentioned answer allows you to cancel all previous tasks, but the fact is that - when you synchronize properly (like in registerTask) - you only need to store the last task because all previous tasks will already be cancelled (by the same registerTask method, which - due to being synchronized - will never execute concurrently). And as far as I know, calling cancel on a Task that has already completed is a no-op.

Moreover, using an ExecutorService over new Thread(task).start() (like Nghia Do proposed) is also a good idea, because Thread creation is expensive.


EDIT: I haven't given your MCVE a try before, sorry. Now that I have, and that I haven't been able to reproduce the behavior, I noticed the following flaws in your MCVE:

  1. You're using Thread.sleep() to simulate calculations, but calling Future.cancel() when inside Thread.sleep() terminates the task immediately because Future.cancel() calls Thread.interrupt() inside, and Thread.sleep() checks for interruption flag, which you most likely don't do in your real code.

  2. You're assigning the result of the calculation to a private field. Since (as mentioned above) the calculation was interrupted in your original MCVE immediately, and probably wasn't interrupted in your real code, in your real code the value of the field could get overwritten by an already canceled task.

  3. I believe that in your real code, you probably do more than just assining the result to the field. In the original MCVE, I wasn't able to reproduce the behavior even after replacing Thread.sleep() with a loop. The reason for this is that the onSucceeded handler does not get called, when the task was canceled (which is good). I believe, though, that in your real code, you may have done something more important (like some GUI update) in Task.call() than what you've shown in MCVE, because otherwise, as far as I understand, you wouldn't have experienced the original problem.

Here's a modification of your MCVE that should not exhibit the original problem.

EDIT: I further modified the MCVE to print cancelation info and also print times of events. This code, however, does not reproduce the behavior the OP wrote about.

public class TaskRace extends Application {

    private final ListView<String> listView = new ListView<>();
    private final Label label = new Label("Nothing selected");
    private final SingleTaskRunner runner = new SingleTaskRunner();

    private final long startMillis = System.currentTimeMillis();

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

    @Override
    public void start(Stage stage) {

        // Simple UI
        VBox root = new VBox(5);
        root.setAlignment(Pos.CENTER);
        root.setPadding(new Insets(10));
        root.getChildren().addAll(listView, label);

        // Populate the ListView
        listView.getItems().addAll(
                "One", "Two", "Three", "Four", "Five"
        );


        // Add listener to the ListView to start the task whenever an item is selected
        listView.getSelectionModel().selectedItemProperty().addListener((observableValue, oldValue, newValue) -> {

            if (newValue != null) {
                // Create the background task
                MyTask task = new MyTask();

                // Update the label when the task is completed
                task.setOnSucceeded(event -> {
                    label.setText(task.getValue());
                    println("Assigned " + task.selectedItem);
                });
                task.setOnCancelled(event -> println("Cancelled " + task.selectedItem));

                runner.runTask(task);
            }

        });

        stage.setScene(new Scene(root));
        stage.show();

    }

    private void println(String string) {
        System.out.format("%5.2fs: %s%n", 0.001 * (System.currentTimeMillis() - startMillis), string);
    }

    private class MyTask extends Task<String> {

        final String selectedItem = listView.getSelectionModel().getSelectedItem();

        @Override
        protected String call() {
            int ms = new Random().nextInt(10000);
            println(String.format("Will return %s in %.2fs", selectedItem, 0.001 * ms));

            // Do long-running task (takes random time)
            long limitMillis = System.currentTimeMillis() + ms;
            while (System.currentTimeMillis() < limitMillis) {
            }

            println("Returned " + selectedItem);
            return "You have selected item: " + selectedItem;
        }
    }
}
Tomasz Linkowski
  • 4,386
  • 23
  • 38
  • This does not appear to make any difference. In my MCVE above, I have replaced the `new Thread(task).start();` line with `runner.runTask(task);` and `runner` is an instance of your `SingleTaskRunner` class. The result is the same as the original question. – Zephyr Jul 17 '18 at 19:40
  • I am confused, again. Other than using a loop instead of `Thread.sleep()`, what changes did you make and why did that work? I can't update my actual program to simply use a loop... – Zephyr Jul 19 '18 at 00:04
  • @Zephyr I'll try to explain. Point 1 concerned the problem with **verifiability** in your *MCVE* (so you absolutely don't have to introduce any loop to your real code). Points 2 and 3 concerned both MCVE and your real code - if, in your real code, you do any *state modifications* to the `Application` instance inside `Task.call()` (in your MCVE, you do it by mutating `labelValue`), please, try to move them to `Task.setOnSucceeded`, and try to use `Task.getValue()` for transferring the calculation result *from* `Task.call()` *to* your `Application` (instead of mutating `Application`s state). – Tomasz Linkowski Jul 19 '18 at 03:36
  • Thank you for trying, but I still have the issue. In fact, in my real app, the tasks never get cancelled and I'm not sure why. I see that `lastTask.cancel()` IS being executed, but my `onCancelled()` method is not. The `onSucceeded()` method is always executed. – Zephyr Jul 19 '18 at 15:11
  • You may check the return value of `lastTask.cancel()` (debugger's your friend here). Probably it's `false`, which means the task has not been canceled because it has already completed. I don't know, maybe you're delegating your computations to some other thread, and the task completes immediately? Until you provide an MCVE that reflects the complexity of your real code much closer, it'll be difficult to help, because it'd involve too much guessing. PS. I updated the modified MCVE in my answer to also print cancelation info. You might try to adapt it further to become more like your real code. – Tomasz Linkowski Jul 19 '18 at 18:07
0

If, for whatever reason, you are unable to make it work in your real use case using cancelation-based solutions (cf. my answer, Slaw's answer, and actually all the other answers), there's one more thing you may try.

The solution proposed here is theoretically worse than any cancelation-based one (because all the redundant calculations will continue until done, thus wasting resources). However, practically, a working solution will be better than a non-working one (provided that it works for you, and that it's the only one that does - so please check Slaw's solution first).


I propose that you forgo cancelation completely, and instead use a simple volatile field to store the value of the last selected item:

private volatile String lastSelectedItem = null;

Upon selecting a new label, you would update this field:

lastSelectedItem = task.selectedItem;

Then, in the on-succeeded event, you would simply check whether you're allowed to assign the result of the computation:

if (task.selectedItem.equals(lastSelectedItem))

Find the entire modified MCVE below:

public class TaskRaceWithoutCancelation extends Application {

    private final ListView<String> listView = new ListView<>();
    private final Label label = new Label("Nothing selected");

    private final long startMillis = System.currentTimeMillis();

    private volatile String lastSelectedItem = null;

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

    @Override
    public void start(Stage stage) {

        // Simple UI
        VBox root = new VBox(5);
        root.setAlignment(Pos.CENTER);
        root.setPadding(new Insets(10));
        root.getChildren().addAll(listView, label);

        // Populate the ListView
        listView.getItems().addAll(
                "One", "Two", "Three", "Four", "Five"
        );


        // Add listener to the ListView to start the task whenever an item is selected
        listView.getSelectionModel().selectedItemProperty().addListener((observableValue, oldValue, newValue) -> {

            if (newValue != null) {
                // Create the background task
                MyTask task = new MyTask();
                lastSelectedItem = task.selectedItem;

                // Update the label when the task is completed
                task.setOnSucceeded(event -> {
                    if (task.selectedItem.equals(lastSelectedItem)) {
                        label.setText(task.getValue());
                        println("Assigned " + task.selectedItem);
                    }
                });

                new Thread(task).start();
            }

        });

        stage.setScene(new Scene(root));
        stage.show();

    }

    private void println(String string) {
        System.out.format("%5.2fs: %s%n", 0.001 * (System.currentTimeMillis() - startMillis), string);
    }

    private class MyTask extends Task<String> {

        final String selectedItem = listView.getSelectionModel().getSelectedItem();

        @Override
        protected String call() {
            int ms = new Random().nextInt(10000);
            println(String.format("Will return %s in %.2fs", selectedItem, 0.001 * ms));

            // Do long-running task (takes random time)
            long limitMillis = System.currentTimeMillis() + ms;
            while (System.currentTimeMillis() < limitMillis) {
            }

            println("Returned " + selectedItem);
            return "You have selected item: " + selectedItem;
        }
    }
}
Tomasz Linkowski
  • 4,386
  • 23
  • 38