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

SettableListenableFuture setException is inconsistent with callbacks under race [SPR-15409] #19972

Closed
spring-issuemaster opened this Issue Apr 3, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

spring-issuemaster commented Apr 3, 2017

Ivan Sopov opened SPR-15409 and commented

In continuation of #19781 and #19766 I decided to test setException method of SettableListenableFuture and received another portion of strange results.

With methods set(value) and setException(new Exception()) executed under race following cases are possible:

  • Both methods return false but SuccessCallback is executed
  • Both methods return false but FailureCallback is executed

I consider these cases as two flavors of the same problem - actually I expect that one of two methods will always return true.

Jcstress-based tests may be found here:
https://github.com/isopov/isopov-jcstress/blob/master/src/main/java/com/sopovs/moradanen/jcstress/spring/SettableListenableFuture3Test.java

Also I tried to reproduce the problem without jcstress to better understand it and make sure that it lies not in my poor understanding of jcstress:
https://github.com/isopov/isopov-jcstress/blob/master/src/main/java/com/sopovs/moradanen/jcstress/spring/SettableListenableFutureMain3Test.java
This test does not reproduce the problem as reliably as jcstress-based one, but sometimes it still reproduces both flavors of the problem in the single run.


Affects: 4.3.7

Issue Links:

  • #19766 SettableListenableFuture may be both set and canceled successfully
  • #19781 SettableListenableFuture may be successfully set with failureCallback executed and success callback ignored

Referenced from: commits 8321f01, 8cb24e0

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Apr 7, 2017

Juergen Hoeller commented

There was indeed a bug in SettableTask.checkCompletingThread: We should only reset the marker after a successful check, otherwise a non-successful check may invalidate it and a subsequent potentially successful check can't find the corresponding marker anymore. I've fixed that in master now and will backport it to 4.3.8.

Note that your test has a race condition of its own: For a valid immediate comparison, each success/successCallback and exception/failCallback pair needs to be checked within a common monitor for the pair, otherwise they may be temporarily out of sync.

synchronized (container.SUCCESS_MONITOR) {
    if (container.result.successCallback && !container.result.success) {
        System.out.println("SuccessCallback without success!");
    }
}
synchronized (container.SUCCESS_MONITOR) {
    // success flag and success callback flag set within the same monitor
    container.result.success = container.future.set("foo");
}

and the same for the setException case, e.g. with a container.FAIL_MONITOR. Alternatively, I guess you could also batch the results and compare all of them at the very end.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Apr 8, 2017

Ivan Sopov commented

I've build current master from sources and all my tests pass - thank you!

Thanks for this catch - indeed Main-based "simple" test reports failures even with the fix and has its own bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.