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

MonoToListenableFutureAdapter.cancel problems [SPR-17336] #21870

Closed
spring-projects-issues opened this issue Oct 4, 2018 · 4 comments
Closed
Assignees
Labels
in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Oct 4, 2018

Ivan Sopov opened SPR-17336 and commented

org.springframework.messaging.support.MonoToListenableFutureAdapter.cancel method is strange.
Consider adapter for the case that cannot be cancelled.

Future<Void> foo = new MonoToListenableFutureAdapter<>(Mono.empty());
System.out.println(foo.cancel(true));
System.out.println(foo.isCancelled());

This prints:

true
false

This clearly violates cancel method javadoc "Subsequent calls to isCancelled will always return true if this method returned true" And I believe cancel should've returned false itself since it should "return false if the task could not be cancelled, typically because it has already completed normally"

Also curent implementation gives result true several times under race if actual cancellation is happening. Consider the following JCStress-based test:

@JCStressTest
@Outcome(id = "true, false", expect = Expect.ACCEPTABLE)
@Outcome(id = "false, true", expect = Expect.ACCEPTABLE)
@Outcome(expect = Expect.FORBIDDEN, desc = "Other cases are forbidden.")
@State
public class MonoToListenableFutureAdapterCancelTest {
    private final Future<Void> future = new MonoToListenableFutureAdapter<>(Mono.fromFuture(new CompletableFuture<>()));
    @Actor
    public void cancel1(ZZ_Result r) {
        r.r1 = future.cancel(true);
    }
    @Actor
    public void cancel2(ZZ_Result r) {
        r.r2 = future.cancel(true);
    }
}

It fails with "true, true" result while result "false, true" and "true, false" are also observed.


Affects: 5.1 GA

Issue Links:

Referenced from: commits c01f350, 928c541, bef22ec

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Oct 4, 2018

Rossen Stoyanchev commented

This type was mostly for internal use, within the ReactorNettyTcpClient until it was made public recently with #21175. Indeed it should not return a hardcoded "true" value so I'm scheduling this for a change.

Can you clarify how you're using and ran into this? Directly or perhaps indirectly via ReactorNettyTcpClient or an @MessageMapping method returning a Mono?

@spring-projects-issues
Copy link
Collaborator Author

Ivan Sopov commented

Sorry - I'm not actually using it. Found it mentioned here - https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/util/concurrent/ListenableFuture.html

@spring-projects-issues
Copy link
Collaborator Author

Rossen Stoyanchev commented

Ok no worries, thanks.

@spring-projects-issues
Copy link
Collaborator Author

Rossen Stoyanchev commented

Note that this class is now in spring-core. It became public only very recently in 5.1 GA, but remained in spring-messaging. We're fixing this in 5.1.1 by putting it where it belongs in org.springframework.util.concurrent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: messaging Issues in messaging modules (jms, messaging) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants