Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi await feature for intentional blocking #1664

Merged
merged 10 commits into from
May 13, 2020
Merged

Multi await feature for intentional blocking #1664

merged 10 commits into from
May 13, 2020

Conversation

danielkec
Copy link
Contributor

There was a request for blocking feature for Multi and Single

Multi.interval(20, TimeUnit.MILLISECONDS, Executors.newSingleThreadScheduledExecutor())
        .limit(5)
        .forEach(sum::addAndGet)
        .await();
Multi.interval(20, TimeUnit.MILLISECONDS, Executors.newSingleThreadScheduledExecutor())
        .limit(5)
        .reduce(Long::sum)
        .await();

I guess this needs some broader discussion, it kind of feels like introducing an antipattern. But the truth is other impls has similar feature and users will be expecting it.

Signed-off-by: Daniel Kec daniel.kec@oracle.com

Copy link
Collaborator

@akarnokd akarnokd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings about blocking operators and since Project Loom is not fixed yet, it's unclear how we can play nice with it as well.

@danielkec danielkec self-assigned this Apr 20, 2020
@danielkec danielkec added reactive Reactive streams and related components enhancement New feature or request labels Apr 20, 2020
@romain-grecourt
Copy link
Contributor

Can you rebase against master ? you're missing one of the checks from the new infra.

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
@danielkec
Copy link
Contributor Author

@tomas-langer had proposed Single implementing CompletionStage which is seems to be quite nicer, so Multi.forEach can returnSingle<Void>, await methods are part of single on one place only and its possible to return Single on the apis where completables are expected

@danielkec danielkec added this to the 2.0.0 milestone Apr 30, 2020
@danielkec danielkec linked an issue Apr 30, 2020 that may be closed by this pull request
@danielkec
Copy link
Contributor Author

Can I ask @romain-grecourt and @tomas-langer for review? @Verdent is blocked by this with #1646 Question is if Single implementing CompletionStage is preferable or not, Imho I like it as it makes all apis returning Single more comfortable.

@romain-grecourt
Copy link
Contributor

@danielkec Yeah, Let's go ahead with Single implementing CompletionStage, we can leverage it in our APIs where we return a CompletionStage.

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
romain-grecourt
romain-grecourt previously approved these changes May 5, 2020
Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
tomas-langer
tomas-langer previously approved these changes May 7, 2020
Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
We may want to revisit the completion stage single, but this is a good first change

@danielkec
Copy link
Contributor Author

Awaits are now preserved when chaining CompletionStage methods:

        testMulti()
                .forEach(sum::addAndGet)
                .whenComplete((aVoid, throwable) -> {})
                .thenRun(() -> {})
                .thenAccept(aVoid -> {})
                .await();

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@danielkec danielkec merged commit 573cd6e into master May 13, 2020
@olotenko
Copy link

This seems like a massive breaking change just to get a wrapper for a timed CompletableFuture.get.

onCancel implementation is incompatible in its behaviour with superclass. Multi.forEach can miss cancellation indefinitely. Dangling Subscribers are possible.

@danielkec
Copy link
Contributor Author

@olotenko pls elaborate, is it order of?:

                    subscription.request(Long.MAX_VALUE);
                    single.onCancel(subscription::cancel);

@olotenko
Copy link

Yes, but don't you see the whole design of the change is rickety?

A plain static method for two flavours of await would serve the purpose. I really don't see the value of rewriting CompletableFuture and changing how all the reactive pipelines everywhere behave.

@romain-grecourt romain-grecourt deleted the multi-await branch May 19, 2020 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request reactive Reactive streams and related components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants