[FIXED] JavaFX task/thread not filling tableview consistently

Issue

So, i need to fill a Table View using a JavaFX thread but the table is being filled only ~70% of time. I am looking at my code and i really can't find where the problem comes from, my guess is that the task is somehow being executed before the data is successfully retrieved/processed from db. Thanks is advance :)

private Executor exec;
private ObservableList<User> cellData = FXCollections.observableArrayList();
.
.
.
public void fillTable(HashMap<String,Object> whereClause){
        Task<List<User>> task = new Task<List<User>>(){
            @Override
            public ObservableList<User> call(){
                cellData.clear();
                cellData.addAll(userRepository.getAll(whereClause));
                userId.setCellValueFactory(new PropertyValueFactory<>("userID"));
                userName.setCellValueFactory(new PropertyValueFactory<>("userName"));
                userMail.setCellValueFactory(new PropertyValueFactory<>("userMail"));
                userPhone.setCellValueFactory(new PropertyValueFactory<>("userPhone"));
                isAdmin.setCellValueFactory(cellData -> {
                    String isAdminAsString = cellData.getValue().isAdmin() ? "Admin" : "Medic";
                    return new ReadOnlyStringWrapper(isAdminAsString);
                });
                isDeleted.setCellValueFactory(cellData -> {
                    String isActiveUser = cellData.getValue().isDeleted() ? "No" : "Yes";
                    return new ReadOnlyStringWrapper(isActiveUser);
                });
                logger.info("Cell values set");
                return cellData;
            }
        };
        exec.execute(task);
        task.setOnFailed(e -> System.out.println(task.getException().getMessage()));
        task.setOnSucceeded(e -> userTable.setItems((ObservableList<User>) task.getValue()));
        logger.info("Fill user Table Task executed");

Solution

You don't give enough context for a proper, fully confident answer, but my guess is you're encountering issues relating to threads. JavaFX is not thread-safe; using the wrong thread to update the UI can lead to undefined behavior, such as the data only appearing ~70% of the time. There's an important rule in JavaFX that you must always follow:

• Never read or write the state of objects that are connected—directly or indirectly—to a live scene graph on a thread other than the JavaFX Application Thread.

Your code does not follow this rule. Inside the call method of your Task you are structurally modifying cellData and setting the cellValueFactory of various TableColumns. This leads to said objects being modified by whatever thread is executing the Task. If the Executor is any hint, that thread is definitely not the JavaFX Application Thread.

I'm not sure why you're setting the cellValueFactory of your TableColumns inside the call method in the first place. The cell value factory is configuration that only needs to be done once—when you create the TableColumn (or shortly thereafter). In other words, configuring the cell value factory in the call method is wrong not just because it happens on a background thread but also because it happens each time you execute the Task. Remove the set-the-cell-value-factory code from the call method and move it, if needed, to where you're creating the TableColumns. If you're using FXML, and the TableColumns are created for you and injected, then the controller's initialize method is a good place for this sort of configuration.

Your cellData list is connected to your TableView, if not at first then definitely after the first successful execution of your Task. Modifying cellData on a background thread will notify the TableView of those changes on the same thread (listeners are invoked on the same thread that made the change). The easy solution is to have your Task return a new List and then update the TableView if successful.

Task<List<User>> task = new Task<List<User>>() {
    @Override protected List<User> call() throws Exception {
        return userRepository.getAll(whereClause);
    }
});
task.setOnSucceeded(event -> userTable.getItems().setAll(task.getValue()));
task.setOnFailed(event -> task.getException().printStackTrace());
exec.execute(task);

The setAll method of ObservableList will first clear the list then add all the elements of the given collection (or array). This is somewhat more efficient than calling clear followed by addAll because it results in only one change event. Also, if you want to continue using cellData you can, assuming you've previously set it as your table's items; just use cellData.setAll(task.getValue()) instead.

---

Regarding the use of:

task.setOnSucceeded(e -> userTable.setItems((ObservableList<User>) task.getValue()));

Since you clearly expect an ObservableList<User> to be returned, you should be using a Task<ObservableList<User>> instead of a Task<List<User>>. This will mean getValue() returns ObservableList<User> and thus the cast becomes unneeded. However, if you follow the advice above, then this is irrelevant.

Answered By - Slaw