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

Mono.share() allow a stream to be canceled #3740

Closed
rgarciapariente opened this issue Mar 5, 2024 · 5 comments
Closed

Mono.share() allow a stream to be canceled #3740

rgarciapariente opened this issue Mar 5, 2024 · 5 comments
Labels
type/documentation A documentation update
Milestone

Comments

@rgarciapariente
Copy link

rgarciapariente commented Mar 5, 2024

We are using Mono::share and if the mono recive a TimeOut exception the mono execution is canceled

Expected Behavior

I expect that a timeout don't cancel a Mono return by share() method.

Actual Behavior

The mono is cancelled

Steps to Reproduce

@Test
void reproCase() {
        var stream = Mono.just("Hola")
                .doOnNext(next -> System.out.println("Learning to say hi: " + next))
                .delayElement(Duration.of(5, ChronoUnit.SECONDS))
                .doOnNext(next -> System.out.println("Say hi: " + next))
                .share();

        System.out.println("Init ...");

        var result = stream
                .subscribeOn(Schedulers.boundedElastic())
                .timeout(Duration.of(1, ChronoUnit.SECONDS))
                .onErrorReturn(TimeoutException.class, "Timeout :(")
                .block();

        System.out.println("Result: " + result);

        stream.subscribe();

        Thread.sleep(10_000);
}

Result:

Init ...
Learning to say hi: Hola
Result: Timeout :(
Learning to say hi: Hola
Say hi: Hola

Note the two "Learning to say ...."

but

@Test
void workingCase() {
        var stream = Mono.just("Hola")
                .doOnNext(next -> System.out.println("Learning to say hi: " + next))
                .delayElement(Duration.of(5, ChronoUnit.SECONDS))
                .doOnNext(next -> System.out.println("Say hi: " + next))
                .share();

        System.out.println("Init ...");

        stream.subscribe();

        var result = stream
                .subscribeOn(Schedulers.boundedElastic())
                .timeout(Duration.of(1, ChronoUnit.SECONDS))
                .onErrorReturn(TimeoutException.class, "Timeout :(")
                .block();

        System.out.println("Result: " + result);

        Thread.sleep(10_000);
}

Result:

Init ...
Learning to say hi: Hola
Result: Timeout :(
Say hi: Hola

Your Environment

  • Reactor version(s) used: 3.4.30
  • JVM version (java -version): Java version: 17.0.4, vendor: GraalVM Community,
@reactorbot reactorbot added the ❓need-triage This issue needs triage, hasn't been looked at by a team member yet label Mar 5, 2024
@chemicL
Copy link
Member

chemicL commented Mar 5, 2024

Hello, @rgarciapariente 👋

I am not sure that I follow.

The first example looks correct

  • The first subscriber cancels its processing upon timeout and doesn't consume the result.
  • The second subscriber comes in next and extracts the result from the shared instance without issue.

This goes according to the javadoc:

Prepare a Mono which shares this Mono result (...)

The second example has only one subscriber, which cancels itself upon timeout. However, meanwhile, the result is delivered from the source to the subscription – which is not cancelled, and can be used again by late subscribers.

To me it looks as if the behaviour is correct. Can you please explain what seems to be the incorrect behaviour?

@chemicL chemicL added status/need-user-input This needs user input to proceed and removed ❓need-triage This issue needs triage, hasn't been looked at by a team member yet labels Mar 5, 2024
@chemicL
Copy link
Member

chemicL commented Mar 5, 2024

Ah. I think I understand what you mean. You're referring to the fact that the source is being reached twice - that the "Learning to say hi: Hola" is printed out twice. Am I correct? That doesn't mean cancellation I believe, but potentially re-subscribing to the source. Is that the actual issue?

@rgarciapariente
Copy link
Author

Many thanks for your fast answer :)

yes, that is. I am interpreting of javadoc "It's worth noting this is an un-cancellable" as meaning that "Learning to say hi: Hello" should not be executed twice in the first sample

@chemicL
Copy link
Member

chemicL commented Mar 6, 2024

I see. Well, this is quite interesting. Indeed, the upstream subscription gets cancelled in case all downstream subscribers are gone.

A recent change: #2756.

Background: #2680 -> the same reasoning as you brought up.

It makes sense to me after looking into the background and the implementation. The current behaviour sounds like something expected. The issue I see now is the misleading documentation. As you can see in the PR, all other javadocs have been aligned, except this one. Probably an omission.

I'll mark this issue as a documentation problem. Is that ok for you @rgarciapariente ?

@chemicL chemicL added type/documentation A documentation update and removed status/need-user-input This needs user input to proceed labels Mar 6, 2024
@chemicL chemicL added this to the 3.4.x Backlog milestone Mar 6, 2024
chemicL added a commit that referenced this issue Mar 6, 2024
Similarly to Flux#share, Mono#share also cancels the source when all
Subscribers have cancelled. This change improves the documentation.

Following #2680 and #2756 there exists a misalignment in the javadoc for
Mono#share method. Since cancelling the source is a fact, the
javadoc is now improved instead of changing the behaviour.

Resolves #3740
chemicL added a commit that referenced this issue Mar 6, 2024
Similarly to Flux#share, Mono#share also cancels the source when all
Subscribers have cancelled. This change improves the documentation.

Following #2680 and #2756 there exists a misalignment in the javadoc for
Mono#share method. Since cancelling the source is a fact, the
javadoc is now improved instead of changing the behaviour.

Resolves #3740
@rgarciapariente
Copy link
Author

I understand and share your argument. Thank you very much for your time

@chemicL chemicL closed this as completed in 3cf8977 Mar 7, 2024
@chemicL chemicL modified the milestones: 3.4.x Backlog, 3.4.36 Mar 7, 2024
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

No branches or pull requests

3 participants