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

[spring-projects-issues]

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:

• SettableListenableFuture may be both set and canceled successfully [SPR-15202] #19766 SettableListenableFuture may be both set and canceled successfully
• SettableListenableFuture may be successfully set with failureCallback executed and success callback ignored [SPR-15216] #19781 SettableListenableFuture may be successfully set with failureCallback executed and success callback ignored

Referenced from: commits 8321f01, 8cb24e0

[spring-projects-issues]

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-projects-issues]

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.