Skip to content
This repository has been archived by the owner on Feb 15, 2021. It is now read-only.

FxAsync no error handling -> future never finishes #23

Closed
Blackdread opened this issue Nov 30, 2018 · 7 comments
Closed

FxAsync no error handling -> future never finishes #23

Blackdread opened this issue Nov 30, 2018 · 7 comments
Assignees
Labels

Comments

@Blackdread
Copy link

public static <T> CompletionStage<T> doOnFxThread(final T element, final Consumer<T> action) {
        CompletableFuture<T> asyncOp = new CompletableFuture<>();
        Platform.runLater(() -> {
            action.accept(element);
            asyncOp.complete(element);
        });
        return asyncOp;
}

public static <T, U> CompletionStage<U> computeOnFxThread(final T element, final Function<T, U> compute) {
        CompletableFuture<U> asyncComputeOp = new CompletableFuture<>();
        Platform.runLater(() -> asyncComputeOp.complete(compute.apply(element)));
        return asyncComputeOp;
}

change to:

public static <T> CompletionStage<T> doOnFxThread(final T element, final Consumer<T> action) {
        final CompletableFuture<T> asyncOp = new CompletableFuture<>();
        Platform.runLater(() -> {
            try {
                action.accept(element);
                asyncOp.complete(element);
            } catch (Exception e) {
                asyncOp.completeExceptionally(e);
            }
        });
        return asyncOp;
    }
public static <T, U> CompletionStage<U> computeOnFxThread(final T element, final Function<T, U> compute) {
        final CompletableFuture<U> asyncComputeOp = new CompletableFuture<>();
        Platform.runLater(() -> {
            try {
                asyncComputeOp.complete(compute.apply(element));
            } catch (Exception e) {
                asyncComputeOp.completeExceptionally(e);
            }
        });
        return asyncComputeOp;
    }
@Tristan971
Copy link
Owner

Oh indeed. Nice catch! Although it stems from me misusing CompletableFuture to begin with. Will fix asap 👍

@Tristan971 Tristan971 self-assigned this Dec 1, 2018
@Tristan971 Tristan971 added the bug label Dec 1, 2018
@Tristan971
Copy link
Owner

Fixed with 9e6c8b2

@Blackdread
Copy link
Author

Yes and it might not be a perfect implementation as if user does not use async (thenAcceptAsync, etc) methods on the returned completableFuture then it will be the FX thread that will run (might make UI stuck if long running task after).

@Tristan971
Copy link
Owner

Tristan971 commented Dec 2, 2018

Sorry what do you mean I don't really follow 😅
Do you mean if the user does follow-up operations without specifying thread? Wouldn't that just wait for the FX thread to be done then delegate follow-up to either the default threadpool of CFuture or whatever they specify ?

Or is the case that using for example #thenAccept (i.e. the non async one) it will keep blocking the FX thread as it was the first executor specifically called ? (have to admit I always use the async ones anyway since if I don't care about asynchronicity I can revert to blocking between all ops).

@Blackdread
Copy link
Author

Yes it will keep blocking the FX thread as it is the one used as the executor.

All non-async methods will be executed on FX thread #thenAccept.
But for all async methods, it will be executed on default threadpool or the executor passed (thenAcceptAsync(Consumer<? super T> action, Executor executor))

Maybe this should be clearly stated in the javadoc of your methods otherwise fx thread may be stuck on those sync methods.
Example:

final ExecutorService executorService = Executors.newSingleThreadExecutor();

    doOnFxThread(...)
                .thenApply(any-> any) // on Fx thread
                .thenApplyAsync(any-> any) // on ForkJoinPool
                .thenApply(any-> any) // on ForkJoinPool
                .thenApplyAsync(any-> any, executorService) // on executorService
                .thenApply(any-> any); // on executorService


@Tristan971
Copy link
Owner

Well that's interesting. Thank you very much for the info and I'll definitely add this to the documentation!

@Tristan971
Copy link
Owner

Thank you again for the info (the more you know... apparently I'm not the only one who got shaded by CompletableFuture's documentation being not the most straightforward I guess) !

Added documentation warnings in b5ca10a 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants