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

Document Mono.zip subscription scheme when Mono<Void> is supplied #3306

Closed
a701440 opened this issue Dec 2, 2022 · 13 comments · Fixed by #3514
Closed

Document Mono.zip subscription scheme when Mono<Void> is supplied #3306

a701440 opened this issue Dec 2, 2022 · 13 comments · Fixed by #3514
Labels
good first issue Ideal for a new contributor, we'll help type/documentation A documentation update
Milestone

Comments

@a701440
Copy link

a701440 commented Dec 2, 2022

The test below fails when using Mono, if you slightly change the code to use Mono it passes.
3 subscribes are expected, but only one is issued.

    @Test
    public void testZip() {
        AtomicInteger cnt = new AtomicInteger();
        Mono<Void> m1 = Mono.create(sink -> {
            cnt.incrementAndGet();
            sink.success();
        });
        Mono<Void> m2 = Mono.create(sink -> {
            cnt.incrementAndGet();
            sink.success();
        });
        Mono<Void> m3 = Mono.create(sink -> {
            cnt.incrementAndGet();
            sink.success();
        });
        Mono<Void> m4 = m1.zipWith(Mono.zip(List.of(m2,m3), objects -> true), (value1, value2) -> value1);
        m4.block();
        assertEquals(3, cnt.get());
    }
@reactorbot reactorbot added the ❓need-triage This issue needs triage, hasn't been looked at by a team member yet label Dec 2, 2022
@a701440
Copy link
Author

a701440 commented Dec 2, 2022

This fails on 3.5.0, but was passing on 3.4.6.

@OlegDokuka
Copy link
Contributor

@a701440 that is expected. MonoZip does not guarantee all sources are subscribed when one of the given completes or errors immediately. What you have seen in 3.4.x line was a design flaw. Consider using Mono.when instead if you need to wait for all sources to be subscribed and completed

@OlegDokuka OlegDokuka added for/stackoverflow Questions are best asked on SO or Gitter type/documentation A documentation update and removed ❓need-triage This issue needs triage, hasn't been looked at by a team member yet for/stackoverflow Questions are best asked on SO or Gitter labels Dec 2, 2022
@OlegDokuka OlegDokuka reopened this Dec 2, 2022
@OlegDokuka OlegDokuka added the good first issue Ideal for a new contributor, we'll help label Dec 2, 2022
@a701440
Copy link
Author

a701440 commented Dec 2, 2022

It's very confusing that the behavior is very different if you change the code to Integer.

@Test
    public void testZip() {
        AtomicInteger cnt = new AtomicInteger();
        Mono<Integer> m1 = Mono.create(sink -> {
            cnt.incrementAndGet();
            sink.success(1);
        });
        Mono<Integer> m2 = Mono.create(sink -> {
            cnt.incrementAndGet();
            sink.success(2);
        });
        Mono<Integer> m3 = Mono.create(sink -> {
            cnt.incrementAndGet();
            sink.success(3);
        });
        Mono<Integer> m4 = m1.zipWith(Mono.zip(List.of(m2,m3), objects -> true), (value1, value2) -> value1);
        m4.block();
        assertEquals(3, cnt.get());
    }

In my actual case it was used with function.apply. When function was void it did not invoke, when function had a return value it did.

@OlegDokuka
Copy link
Contributor

OlegDokuka commented Dec 5, 2022

@a701440 That is expected!

As I stated above, zip is designed to join corresponding values.
By design, it waits for all sources to emit an onNext signal (the last case corresponds to the expectation).

In case one of the sources is empty, zip unsubscribes from the other (if was subscribed before).
To emphasize one more time, what you have seen before was misbehaving.

@OlegDokuka
Copy link
Contributor

what we need to document here is an alternative approach for zipping using toSingleOptional().

@RomaKudryavtsev
Copy link
Contributor

Hi there! I will be glad to investigate and document this issue. Please let me know if it is still relevant.

@OlegDokuka
Copy link
Contributor

@RomaKudryavtsev just do it!

@RomaKudryavtsev
Copy link
Contributor

RomaKudryavtsev commented Jun 15, 2023

@OlegDokuka , in javadoc to zip it is stated that: "An error or empty completion of any source will cause other sources to be cancelled and the resulting Mono to immediately error or complete, respectively." I believe this what you are referring to in your responses above.
At the same time, the below test case will be passed:

    @Test
    public void testZipVoid() {
        AtomicInteger cnt = new AtomicInteger();
        Mono<Void> mono1 = Mono.create(sink -> {
            cnt.incrementAndGet();
            sink.success();
        });
        Mono<Void> mono2 = Mono.create(sink -> {
            cnt.incrementAndGet();
            sink.success();
        });
        Mono<Void> zippedMono = Mono.zip(mono1, mono2, (v1, v2) -> v1).log();
        zippedMono.block();
        assertEquals(2, cnt.get());
    }

Why would it pass? According to javadoc, I would say that the logic has to be as follows: mono1 completes empty, therefore zippedMono should complete immediately (i.e. without waiting for mono2). But it follows from the test case that it is quite opposite - both mono1 and mono2 are completed. Or maybe it would be proper to say that indeed .zip() simply does not guarantee that all of the publishers will be completed (meaning that in fact they might be completed). Sorry for being boring - I just want to understand the issue properly in order to add this to documentation. Also just want to note that I am using 3.5.0 version

@OlegDokuka
Copy link
Contributor

@RomaKudryavtsev your example is rather coincidence which happens now but in the future it may be different. The correct answer - there is no guarantee

@chemicL chemicL changed the title Mono.zip does not properly subscribe when used with Mono<Void> Document Mono.zip subscription scheme when Mono<Void> is supplied Jun 20, 2023
@chemicL
Copy link
Member

chemicL commented Jun 20, 2023

I updated the title of the issue to reflect the current scope of work.

@RomaKudryavtsev
Copy link
Contributor

@chemicL , hi! I will do it this week - my input will be based on above reply from @OlegDokuka

RomaKudryavtsev added a commit to RomaKudryavtsev/reactor-core that referenced this issue Jun 22, 2023
Documented that upon supply of Mono<Void> it is not guaranteed that the resulted zipped Mono will not complete immediately upon empty completion of Mono<Void>.
Fixes reactor#3306
Author: Roman Zandler <rmnzndlr@gmail.com>
@RomaKudryavtsev
Copy link
Contributor

@chemicL , @OlegDokuka - please let me know should you have any comments/mark-up.

RomaKudryavtsev added a commit to RomaKudryavtsev/reactor-core that referenced this issue Jun 24, 2023
…ers (reactor#3306)

Added emphasis that the reason for the discussed behavior lies not in the use of Void, but in the empty completion itself.
Fixes reactor#3306
Author: Roman Zandler <rmnzndlr@gmail.com>
RomaKudryavtsev added a commit to RomaKudryavtsev/reactor-core that referenced this issue Jun 27, 2023
…ers (reactor#3306)

Added emphasis on the act of subscribing.
Fixes reactor#3306
Author: Roman Zandler <rmnzndlr@gmail.com>
chemicL pushed a commit that referenced this issue Jun 29, 2023
)

Improves the documentation to provide an explanation about subscription
behaviour of the zip operator in case any provided source completes without
producing items.

Resolves #3306.
@chemicL chemicL added this to the 3.5.8 milestone Jun 29, 2023
@chemicL
Copy link
Member

chemicL commented Jun 29, 2023

Thanks @RomaKudryavtsev and congrats on your first contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Ideal for a new contributor, we'll help type/documentation A documentation update
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants