Skip to content

fixes breaking change for fromFuture source cancellation#3252

Merged
OlegDokuka merged 3 commits into3.4.xfrom
bugfix/3.4.x-completable-future
Oct 28, 2022
Merged

fixes breaking change for fromFuture source cancellation#3252
OlegDokuka merged 3 commits into3.4.xfrom
bugfix/3.4.x-completable-future

Conversation

@OlegDokuka
Copy link
Copy Markdown
Contributor

@OlegDokuka OlegDokuka commented Oct 26, 2022 •
edited
Loading

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

closes #3235

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

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
public void cancel() {
super.cancel();
if (future instanceof Future) {
if (cancel) {
Copy link
Copy Markdown
Member

@chemicL chemicL Oct 26, 2022

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
Copy Markdown
Contributor Author

@OlegDokuka OlegDokuka Oct 26, 2022

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
Copy Markdown
Contributor Author

@OlegDokuka OlegDokuka Oct 26, 2022

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
Copy Markdown
Contributor

@rstoyanchev rstoyanchev left a comment •
edited
Loading

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
Copy Markdown
Contributor Author

OlegDokuka commented Oct 26, 2022

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
Copy Markdown

benwiles1 commented Oct 26, 2022

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
Copy Markdown

benwiles1 commented Oct 26, 2022 •
edited
Loading

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.

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

OlegDokuka commented Oct 28, 2022

@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
Copy Markdown

reactorbot commented Oct 28, 2022

@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 pushed a commit that referenced this pull request Oct 28, 2022
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
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
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

@rstoyanchev rstoyanchev Awaiting requested review from rstoyanchev

@chemicL chemicL Awaiting requested review from chemicL

1 more reviewer

@simonbasle simonbasle simonbasle approved these changes

Reviewers whose approvals may not affect merge requirements

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.

6 participants

@OlegDokuka @benwiles1 @reactorbot @rstoyanchev @chemicL @simonbasle