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

[doc] Document usage of Mono.zip() along with Mono<Void> (#3306) #3514

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

RomaKudryavtsev
Copy link
Contributor

Documented that upon supply of Mono it is not guaranteed that the resulted zipped Mono will not complete immediately upon empty completion of Mono. Fixes #3306
Author: Roman Zandler rmnzndlr@gmail.com

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 RomaKudryavtsev requested a review from a team as a code owner June 22, 2023 10:20
@chemicL
Copy link
Member

chemicL commented Jun 23, 2023

I'm a bit skeptical of the use of the combinator in the examples, and especially the return of Void value in the Function. I have a feeling the explanation can be simpler. I believe it's also my mistake to assume we're documenting behaviour in case of Void, but narrowing it down to this specific case makes the comprehension more difficult. The principle applies to any type.

Consider the following alternative, without using Void:

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

@Test
public void testZipEmptyCompletionOneSubscribed() {
	AtomicInteger cnt = new AtomicInteger();
	Mono<Integer> mono1 = Mono.create(sink -> {
		cnt.incrementAndGet();
		sink.success();
	});
	Mono<Integer> mono2 = Mono.create(sink -> {
		cnt.incrementAndGet();
		sink.success();
	});
	Mono<Integer> mono3 = Mono.create(sink -> {
		cnt.incrementAndGet();
		sink.success();
	});
	Mono<Integer> zippedMono = Mono.zip(mono1, Mono.zip(mono2, mono3, (v1, v2) -> v1), (v1, v2) -> v1);
	zippedMono.subscribe();
	assertEquals(1, cnt.get());
}

@Test
public void testZipOptionalAllSubscribed() {
	AtomicInteger cnt = new AtomicInteger();
	Mono<Integer> mono1 = Mono.create(sink -> {
		cnt.incrementAndGet();
		sink.success();
	});
	Mono<Integer> mono2 = Mono.create(sink -> {
		cnt.incrementAndGet();
		sink.success();
	});
	Mono<Integer> mono3 = Mono.create(sink -> {
		cnt.incrementAndGet();
		sink.success();
	});
	Mono<Optional<Integer>> zippedMono =
			Mono.zip(
					mono1.singleOptional(),
					Mono.zip(mono2.singleOptional(), mono3.singleOptional(), (v1, v2) -> v1),
					(v1, v2) -> v1);
	zippedMono.subscribe();
	assertEquals(3, cnt.get());
}

@RomaKudryavtsev
Copy link
Contributor Author

RomaKudryavtsev commented Jun 23, 2023

@chemicL , many thanks for your feedback! So, in fact, the problem lies not in the use of Void, but in the empty completion itself (and such behavior is pretty same with any type - e.g., Mono Integer as well). I will refactor then my input this weekend.

…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
Copy link
Contributor Author

@chemicL , I have committed changes in accordance with your above suggestions. Please let me know if anything else might be improved.

----
====

While in this case the resulting `zippedMono` does not complete immediately upon empty completion of `mono1`, such behaviour is not guaranteed for all cases. For instance, consider the following test case:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
While in this case the resulting `zippedMono` does not complete immediately upon empty completion of `mono1`, such behaviour is not guaranteed for all cases. For instance, consider the following test case:
While in this case the resulting `zippedMono` subscribes to both `mono1` and `mono2`, such behaviour is not guaranteed for all cases. For instance, consider the following test case:

Copy link
Member

Choose a reason for hiding this comment

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

I think the language mentioning completing immediately is not the essence here and can be confusing. Whether it completes before or after is an implementation detail, but the user is most probably interested in the act of subscribing and processing the sources.

}
----
====
In this case upon empty completion of `mono1`, `zippedMono` completes immediately.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In this case upon empty completion of `mono1`, `zippedMono` completes immediately.
In this case upon empty completion of `mono1`, `zippedMono` completes immediately and does not subscribe to `mono2` and `mono3`.

====
In this case upon empty completion of `mono1`, `zippedMono` completes immediately.

In cases where `zip` operator is used to combine empty-completed publishers, it is therefore not guaranteed that the resulting publisher will complete only upon completion of all publishers to be combined. In other words, while in some cases this behaviour might be different, it is not guaranteed that the resulting publisher will not complete immediately upon empty completion of the first publisher to be zipped.
Copy link
Member

Choose a reason for hiding this comment

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

Please consider rewriting this according to the above suggestions around subscription and not completion.

@chemicL
Copy link
Member

chemicL commented Jun 26, 2023

I made some more comments. Thanks for revisiting once more. I think the essence here is - completion of one source without emitting an item can influence the act of subscribing to other sources in undefined ways. It can either subscribe and cancel / discard / never subscribe. As far as I understood the risk is not subscribing to most users and the goal here is to show how to ensure subscription to all the sources.

@RomaKudryavtsev
Copy link
Contributor Author

@chemicL , many thanks for your feedback! I will revert with revisions in the following 2 days.

…ers (reactor#3306)

Added emphasis on the act of subscribing.
Fixes reactor#3306
Author: Roman Zandler <rmnzndlr@gmail.com>
@RomaKudryavtsev
Copy link
Contributor Author

@chemicL , hi! I have made some changes in accordance with your suggestions. Hopefully, now the description is clear enough.

@chemicL chemicL added the type/documentation A documentation update label Jun 29, 2023
@chemicL chemicL added this to the 3.5.8 milestone Jun 29, 2023
@chemicL chemicL merged commit 6431bcb into reactor:main Jun 29, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/documentation A documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document Mono.zip subscription scheme when Mono<Void> is supplied
2 participants