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.

Sign up for GitHub

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

Jump to bottom

fixes breaking change for fromFuture source cancellation #3252

Merged
merged 3 commits into from Oct 28, 2022
Merged

fixes breaking change for fromFuture source cancellation #3252

merged 3 commits into from Oct 28, 2022

Conversation

OlegDokuka
Copy link
Contributor

@OlegDokuka OlegDokuka commented Oct 26, 2022 •
edited

This PR rollbacks changes introduces by #3146 and reintroduce through a new configurable overload

closes #3235

Signed-off-by: Oleh Dokuka odokuka@vmware.com

@OlegDokuka
fixes breaking change for fromFuture source cancellation
e6d081b 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
@OlegDokuka OlegDokuka added type/enhancement A general enhancement for/user-attention This issue needs user attention (feedback, rework, etc...) labels Oct 26, 2022
@OlegDokuka OlegDokuka added this to the 3.4.25 milestone Oct 26, 2022
@OlegDokuka OlegDokuka requested a review from a team as a code owner October 26, 2022 10:28
chemicL
chemicL reviewed Oct 26, 2022
View reviewed changes
reactor-core/src/main/java/reactor/core/publisher/MonoCompletionStage.java Outdated
@@ -52,7 +60,7 @@ public void subscribe(CoreSubscriber<? super T> actual) {
@Override
public void cancel() {
super.cancel();
if (future instanceof Future) {
if (cancel) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe the instanceof check is still required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is impossible that cancel == true and !future instanceof Future. The public API allows only CompletableFuture to have the cancel flag configurable. in all other cases, it is false. This, checking instance of in addition to cancel is true will be overhead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check the following code -> https://github.com/reactor/reactor-core/pull/3252/files#diff-e815d2e7a55f9f17006048ff085897e713143567345688c33e9d64b19f911f06R51

rstoyanchev
rstoyanchev reviewed Oct 26, 2022
View reviewed changes
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment •
edited

Choose a reason for hiding this comment

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

This is a regression, no doubt, but arguably passing on the cancellation signal is the intuitive default behavior that probably should have been the starting point.

From the point of view of the common case, maybe the new fromFuture overloaded method should call the flag suppressCancel so that true matches the main reason to use the method. You could also consider scaling back from two to one overloaded methods? We don't need full convenience for the uncommon case.

I've asked for clarification on the use case under #3235 that may provide further context related to this.

@OlegDokuka
Copy link
Contributor Author

This is a regression, no doubt, but arguably passing on the cancellation signal is the intuitive default behavior that probably should have been the starting point.

From that point of view of the common case, the new fromFuture overload could call this flag suppressCancel where a value of true reflects the main reason for using the overload. It would also mean scaling back on having two overloaded methods. Perhaps just one is sufficient? We don't need to aim for full convenience of an uncommon case.

I've asked for clarification on the use case under #3235 that may provide further context related to this.

Sounds good! Thanks.

@benwiles1
Copy link

Mono.fromCompletionStage docs might want to be updated to also mention that Mono.fromFuture is an alternative that can be used to get the cancel logic.
as in suggest Mono.fromFuture(stage.toCompletableFuture(), true)

@benwiles1
Copy link

benwiles1 commented Oct 26, 2022 •
edited

although maybe not very obvious this change does make it so that Mono.fromCompletionStage(future) doesn't have cancel logic, instead of having cancel logic based on the runtime type of future. which I think is overall a good thing even if Mono.fromFuture(future) is modified to have cancel logic again.

@OlegDokuka
adds overload to suppress cancellation; updates docs
436f61f 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
@OlegDokuka
improves docs
2b24aa7 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
@OlegDokuka
Copy link
Contributor Author

@rstoyanchev @benwiles1 updated the PR as you both suggested. Feel free to have a look

@OlegDokuka OlegDokuka merged commit 7068604 into 3.4.x Oct 28, 2022
@OlegDokuka OlegDokuka deleted the bugfix/3.4.x-completable-future branch October 28, 2022 16:39
@reactorbot
Copy link

@OlegDokuka this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to main 🙇

OlegDokuka added a commit that referenced this pull request Oct 28, 2022
@OlegDokuka
Merge #3252 into 3.5.0
4f3a71f 
Signed-off-by: Oleh Dokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
@chemicL chemicL mentioned this pull request Nov 2, 2022
Closed
@simonbasle simonbasle mentioned this pull request Nov 3, 2022
Merged
simonbasle added a commit that referenced this pull request Nov 4, 2022
@simonbasle
Improve Mono fromFuture/fromCompletionStage javadocs (#3272)
e8406cd 
This commit revises javadocs of Mono fromFuture and fromCompletionStage
methods to better reflect the cancellation behavior.

Fixes #3252.
chemicL pushed a commit that referenced this pull request Mar 7, 2023
@OlegDokuka @chemicL
fixes breaking change for fromFuture source cancellation (#3252)
b99e32c 
this commits fixes misleading documentation introduced after #3146. Also, this commit adds extra methods overloads which allow suppression of the cancellation propagation to the observed `Future`
chemicL pushed a commit that referenced this pull request Mar 7, 2023
@simonbasle @chemicL
Improve Mono fromFuture/fromCompletionStage javadocs (#3272)
7dc3d0b 
This commit revises javadocs of Mono fromFuture and fromCompletionStage
methods to better reflect the cancellation behavior.

Fixes #3252.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonbasle simonbasle simonbasle approved these changes

@rstoyanchev rstoyanchev Awaiting requested review from rstoyanchev

@chemicL chemicL Awaiting requested review from chemicL

Assignees
No one assigned
Labels
for/user-attention This issue needs user attention (feedback, rework, etc...) type/enhancement A general enhancement
Projects
None yet
Milestone
3.4.25
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@OlegDokuka @benwiles1 @reactorbot @rstoyanchev @chemicL @simonbasle