-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fixes breaking change for fromFuture
source cancellation
#3252
Conversation
Signed-off-by: Oleh Dokuka <odokuka@vmware.com> Signed-off-by: OlegDokuka <odokuka@vmware.com>
@@ -52,7 +60,7 @@ public void subscribe(CoreSubscriber<? super T> actual) { | |||
@Override | |||
public void cancel() { | |||
super.cancel(); | |||
if (future instanceof Future) { | |||
if (cancel) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
Sounds good! Thanks. |
|
although maybe not very obvious this change does make it so that |
Signed-off-by: Oleh Dokuka <odokuka@vmware.com> Signed-off-by: OlegDokuka <odokuka@vmware.com>
Signed-off-by: Oleh Dokuka <odokuka@vmware.com> Signed-off-by: OlegDokuka <odokuka@vmware.com>
@rstoyanchev @benwiles1 updated the PR as you both suggested. Feel free to have a look |
@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 |
Signed-off-by: Oleh Dokuka <odokuka@vmware.com> Signed-off-by: OlegDokuka <odokuka@vmware.com>
This commit revises javadocs of Mono fromFuture and fromCompletionStage methods to better reflect the cancellation behavior. Fixes #3252.
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`
This commit revises javadocs of Mono fromFuture and fromCompletionStage methods to better reflect the cancellation behavior. Fixes #3252.
This PR rollbacks changes introduces by #3146 and reintroduce through a new configurable overload
closes #3235
Signed-off-by: Oleh Dokuka odokuka@vmware.com