Non-Blocking Thread-Safe Counter for JavaFX

Question

I am trying to implement a thread-safe solution to keep a count of successful tasks that have been completed, which will ultimately get bound to label displayed on the UI. However, when I use the AtomicInteger below it locks up my UI when the tasks start running, however, if I remove all AtomicInteger refs everything works fine. Is there a non-blocking, thread-safe way which this can be accomplished?

public void handleSomeButtonClick(){
    if(!dataModel.getSomeList().isEmpty()) {
        boolean unlimited = false;
        int count = 0;
        AtomicInteger successCount = new AtomicInteger(0);

        if(countSelector.getValue().equalsIgnoreCase("Unlimited"))
            unlimited = true;
        else
            count = Integer.parseInt(countSelector.getValue());

        while(unlimited || successCount.get() < count) {
            Task task = getSomeTask();
            taskExecutor.submit(task);
            task.setOnSucceeded(event -> {
                if (task.getValue())
                    log.info("Successfully Completed Task | Total Count: " + successCount.incrementAndGet());
                else
                    log.error("Failed task");
            });
        }
    }
}

Answer 1

Your loop waits for a certain number of tasks to be completed. It may even be an infinite loop.

This is not a good idea:

• You block the calling thread which seems to be the JavaFX application thread.
• You don't have any control of how many tasks are submitted. count could be 3, but since you only schedule the tasks in the loop, 1000 or more tasks could be created&scheduled before the first one completes.

Furthermore if you use onSucceeded/onFailed, you don't need to use AtomicInteger or any similar kind of synchronisation, since those handlers all run on the JavaFX application thread.

Your code could be rewritten like this:

private int successCount;

private void scheduleTask(final boolean unlimited) {
    Task task = getSomeTask();
    task.setOnSucceeded(event -> {
        // cannot get a Boolean from a raw task, so I assume the task is successfull iff no exception happens
        successCount++;
        log.info("Successfully Completed Task | Total Count: " + successCount);
        if (unlimited) {
            // submit new task, if the number of tasks is unlimited
            scheduleTask(true);
        }
    });
    // submit new task on failure
    task.setOnFailed(evt -> scheduleTask(unlimited));
    taskExecutor.submit(task);
}

public void handleSomeButtonClick() {
    if(!dataModel.getSomeList().isEmpty()) {
        successCount = 0;
        final boolean unlimited;
        final int count;

        if(countSelector.getValue().equalsIgnoreCase("Unlimited")) {
            unlimited = true;
            count = 4; // set limit of number of tasks submitted to the executor at the same time
        } else {
            count = Integer.parseInt(countSelector.getValue());
            unlimited = false;
        }

        for (int i = 0; i < count; i++) {
            scheduleTask(unlimited);
        }
    }
}

Note: This code runs the risk of handleButtonClick being clicked multiple times before the previous tasks have been completed. You should either prevent scheduling new tasks before the old ones are completed or use some reference type containing an int instead for the count, create this object in handleSomeButtonClick and pass this object to scheduleTask.