Concurrency problem in DeferredResult: potential double execution of handleResult [SPR-14978]

[spring-projects-issues]

yan.zhang opened SPR-14978 and commented

!bug.jpg|thumbnail!

It looks like a concurrency bug.

"this.resultHandler.handleResult(this.result);" may be executed twice.

I think it shoule be:

    private boolean setResultInternal(Object result) {
        synchronized (this) {
            if (isSetOrExpired()) {
                return false;
            }
            this.result = result;

            if (this.resultHandler != null) {
                this.resultHandler.handleResult(this.result);
            }
        }
        return true;
    }

---

Affects: 3.2.17, 4.2.8, 4.3.4, 5.0 M3

Reference URL: https://github.com/spring-projects/spring-framework/blob/v5.0.0.M3/spring-web/src/main/java/org/springframework/web/context/request/async/DeferredResult.java

Attachments:

• bug.jpg (150.03 kB)
• exm1.jpg (64.58 kB)
• exm2.png (89.75 kB)
• exm3.jpg (73.40 kB)

Issue Links:

• Deadlocks with DeferredResult timeout handling on Tomcat [SPR-10485] #15118 Deadlocks with DeferredResult timeout handling on Tomcat
• DeferredResult not thread-safe for isSetOrExpired call [SPR-13451] #18031 DeferredResult not thread-safe for isSetOrExpired call

Backported to: 4.2.9, 3.2.18

[spring-projects-issues]

Juergen Hoeller commented

This looks like a side effect of the deadlock-avoiding fix for #15118: The code used to be the way you suggested it before but the overly strong lock for the handleResult callback led to a potential deadlock in Tomcat.

I guess we need to find a looser way of enforcing a single handleResult callback, e.g. using a volatile variable.

[spring-projects-issues]

yan.zhang commented

thx.

Sorry for my poor english.

I still hava some confusion about it.

It looks like the code in DeferredResult still will lead to a potential deadlock in Tomcat at present.

!exm1.jpg|thumbnail!
the code in DeferredResult.java
!exm2.png|thumbnail!
the code in WebAsyncManager.java
!exm3.jpg|thumbnail!
the code in DeferredResult.java

thread 1: (1)startAsyncProcessing                     (4)setResultHandler
thread 2:                                    (3)DeferredResult.setResult  
thread 3 (handle timeout):  (2)handleTimeout                   (5)setResultInternal

thread 1:setResultHandler(), holding deferredResult lock , requiring tomcat lock
thread 3:setResultInternal(), holding tomcat lock, requiring deferredResult lock

@Juergen Hoeller

[spring-projects-issues]

Juergen Hoeller commented

I've revised DeferredResult to consistently make result handling decisions (including resultHandler != null checks) within its synchronized blocks but actually invoke handleResult outside, in both setResultInternal and setResultHandler. This should avoid any deadlock potential while still making sure that handleResult is only invoked by either setResultInternal or setResultHandler but never both.

[spring-projects-issues]

Rossen Stoyanchev commented

Looks much better this way. I'm wondering if it's worth moving the timeout callback after setting the "timeout result" in a finally block. It is only a callback that can't influence or alter timeout handling. Either way a try-finally might be good to ensure setResultInternal is reached.

[spring-projects-issues]

Juergen Hoeller commented

Hmm, onTimeout's documentation says: "It may invoke setResult or setErrorResult to resume processing." With the current setResultInternal call after the timeout callback invocation, we'd let that sort of custom result happen, with a timeoutResult effectively being ignored when any other result was set before... whereas if we turn it around, a specified timeoutResult will always win over any result the callback might set. So it looks like we'll rather go with a try-finally for the existing sequence?

[spring-projects-issues]

Rossen Stoyanchev commented

Oops, good point indeed that the callback can set the DeferredResult (even says so in onTimeout :)). So yes it needs to stay as it is with a try-finally for the existing sequence. One more point though that if setResulnternal(timeoutResult) does succeed the interceptor method should return false. There is no point to allow other interceptors down the chain.