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

Make Single.concat(Publisher) behavior consistent on cancel #2383

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

idelpivnitskiy
Copy link
Member

@idelpivnitskiy idelpivnitskiy commented Sep 30, 2022

Motivation:

Behavior of Single.concat(Publisher) is different compare to all other concat variants: Single.concat(Completable),
Single.concat(Single), Completable.concat(Completable), Completable.concat(Single), Completable.concat(Publisher), Publisher.concat(Completable), Publisher.concat(Single), Publisher.concat(Publisher). It does not subscribe to the next source to propagate cancellation if onSuccess is delivered after cancel.

Modifications:

  • Make tests for all concat valiants consistent in regards to cancellation;
  • Modify Single.concat(Publisher) behavior to subscribe in case onSuccess is delivered after cancel;

Result:

Behavior of Single.concat(Publisher) is consistent with all other concat variants.

Motivation:

Behavior of `Single.concat(Publisher)` is different compare to all other
`concat` variants: `Single.concat(Completable)`,
`Single.concat(Single)`, `Completable.concat(Completable),
`Completable.concat(Single)`, `Completable.concat(Publisher)`,
`Publisher.concat(Completable)`, `Publisher.concat(Single)`,
`Publisher.concat(Publisher)`. It does not subscribe to the next source
to propagate cancellation if `onSuccess` is delivered after `cancel`.

Modifications:

- Make tests for all `concat` valiants consistent in regards to
cancellation;
- Modify `Single.concat(Publisher)` behavior to subscribe in case
`onSuccess` is delivered after `cancel`;

Result:

Behavior of `Single.concat(Publisher)` is consistent with all other
`concat` variants.
@idelpivnitskiy idelpivnitskiy self-assigned this Sep 30, 2022
@idelpivnitskiy idelpivnitskiy marked this pull request as ready for review September 30, 2022 22:14
@idelpivnitskiy
Copy link
Member Author

Decided to open a PR now as it shouldn't cause merge conflicts with #2381.

@idelpivnitskiy
Copy link
Member Author

@Scottmitch rebased after your changes, ready for review

@@ -142,7 +144,7 @@ private void onErrorPropagateCancel(Throwable t) {

@Override
public final void onComplete() {
if (propagateCancel) {
if (propagateCancel || !deferSubscribe()) {
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't expecting the changes described in this PR to be made here. for example I was expecting the SingleSubscriber.onSuccess(..) methods such that we subscribe eve when completed if CANCELLED (perhaps conditionally?)

thing to watch out for is if propagateCancel we may subscribe to both sources concurrently, and mayBeResultUpdater is used to ensure we only terminate a single time.

can we make the termination probation (on error / on complete) is consistent.

@Scottmitch
Copy link
Member

@idelpivnitskiy - should we close this or do you still think we need this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants