DeferredResult not thread-safe for isSetOrExpired call [SPR-13451]

[spring-projects-issues]

Jamie Hodkinson opened SPR-13451 and commented

The use of DeferredResult is usually from other threads than the one it was created on.

Clients may call isSetOrExpired to check the state of the result: however this may return stale information as it has no synchronization, unlike the setResultInternal method, which synchronizes.

---

Affects: 3.2.14, 4.1.7, 4.2.1

Issue Links:

• Deadlocks with DeferredResult timeout handling on Tomcat [SPR-10485] #15118 Deadlocks with DeferredResult timeout handling on Tomcat
• Make result-data accessible in onCompletion Event in DeferredResult [SPR-10603] #15232 Make result-data accessible in onCompletion Event in DeferredResult
• Concurrency problem in DeferredResult: potential double execution of handleResult [SPR-14978] #19544 Concurrency problem in DeferredResult: potential double execution of handleResult

Referenced from: commits 73a2407, ae0d945, 045016e

Backported to: 4.1.8, 3.2.15

[spring-projects-issues]

Juergen Hoeller commented

It should be sufficient to declare the result and expired fields as volatile here, and to consistently access them with one reference per call only (which requires slight recoding of getResult). Full synchronization is only really necessary for result handler invocations.

Juergen

[spring-projects-issues]

Jamie Hodkinson commented

Agreed. Not sure what the policy is here for synchronization, but given this class may be subclassed, synchronizing on the instance of this class rather than a private lock could cause problems if its subclasses (or even other classes) do the same.

[spring-projects-issues]

Juergen Hoeller commented

Indeed, in particular when trying to check the current state while another thread holds a full lock, volatile fields are preferable. This fix should not cause any regressions compared to the previous code, just make visibility of those fields more consistent.

Juergen