diff --git a/spring-web/src/main/java/org/springframework/web/context/request/async/DeferredResult.java b/spring-web/src/main/java/org/springframework/web/context/request/async/DeferredResult.java index 5ce987149130..ba7842661444 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/async/DeferredResult.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/async/DeferredResult.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -43,6 +43,7 @@ * is added to a {@link PriorityQueue} it is handled in the correct order. * * @author Rossen Stoyanchev + * @author Juergen Hoeller * @author Rob Winch * @since 3.2 */ @@ -65,7 +66,7 @@ public class DeferredResult { private volatile Object result = RESULT_NONE; - private volatile boolean expired; + private volatile boolean expired = false; /** @@ -164,24 +165,39 @@ public void onCompletion(Runnable callback) { */ public final void setResultHandler(DeferredResultHandler resultHandler) { Assert.notNull(resultHandler, "DeferredResultHandler is required"); + // Immediate expiration check outside of the result lock + if (this.expired) { + return; + } + Object resultToHandle; synchronized (this) { - this.resultHandler = resultHandler; - if (this.result != RESULT_NONE && !this.expired) { - try { - this.resultHandler.handleResult(this.result); - } - catch (Throwable ex) { - logger.trace("DeferredResult not handled", ex); - } + // Got the lock in the meantime: double-check expiration status + if (this.expired) { + return; } + resultToHandle = this.result; + if (resultToHandle == RESULT_NONE) { + // No result yet: store handler for processing once it comes in + this.resultHandler = resultHandler; + return; + } + } + // If we get here, we need to process an existing result object immediately. + // The decision is made within the result lock; just the handle call outside + // of it, avoiding any deadlock potential with Servlet container locks. + try { + resultHandler.handleResult(resultToHandle); + } + catch (Throwable ex) { + logger.debug("Failed to handle existing result", ex); } } /** * Set the value for the DeferredResult and handle it. * @param result the value to set - * @return "true" if the result was set and passed on for handling; - * "false" if the result was already set or the async request expired + * @return {@code true} if the result was set and passed on for handling; + * {@code false} if the result was already set or the async request expired * @see #isSetOrExpired() */ public boolean setResult(T result) { @@ -189,15 +205,32 @@ public boolean setResult(T result) { } private boolean setResultInternal(Object result) { + // Immediate expiration check outside of the result lock + if (isSetOrExpired()) { + return false; + } + DeferredResultHandler resultHandlerToUse; synchronized (this) { + // Got the lock in the meantime: double-check expiration status if (isSetOrExpired()) { return false; } + // At this point, we got a new result to process this.result = result; + resultHandlerToUse = this.resultHandler; + if (resultHandlerToUse == null) { + // No result handler set yet -> let the setResultHandler implementation + // pick up the result object and invoke the result handler for it. + return true; + } + // Result handler available -> let's clear the stored reference since + // we don't need it anymore. + this.resultHandler = null; } - if (this.resultHandler != null) { - this.resultHandler.handleResult(this.result); - } + // If we get here, we need to process an existing result object immediately. + // The decision is made within the result lock; just the handle call outside + // of it, avoiding any deadlock potential with Servlet container locks. + resultHandlerToUse.handleResult(result); return true; } @@ -206,8 +239,9 @@ private boolean setResultInternal(Object result) { * The value may be an {@link Exception} or {@link Throwable} in which case * it will be processed as if a handler raised the exception. * @param result the error result value - * @return "true" if the result was set to the error value and passed on for - * handling; "false" if the result was already set or the async request expired + * @return {@code true} if the result was set to the error value and passed on + * for handling; {@code false} if the result was already set or the async + * request expired * @see #isSetOrExpired() */ public boolean setErrorResult(Object result) { @@ -219,19 +253,28 @@ final DeferredResultProcessingInterceptor getInterceptor() { return new DeferredResultProcessingInterceptorAdapter() { @Override public boolean handleTimeout(NativeWebRequest request, DeferredResult deferredResult) { - if (timeoutCallback != null) { - timeoutCallback.run(); + boolean continueProcessing = true; + try { + if (timeoutCallback != null) { + timeoutCallback.run(); + } } - if (DeferredResult.this.timeoutResult != RESULT_NONE) { - setResultInternal(timeoutResult); + finally { + if (timeoutResult != RESULT_NONE) { + continueProcessing = false; + try { + setResultInternal(timeoutResult); + } + catch (Throwable ex) { + logger.debug("Failed to handle timeout result", ex); + } + } } - return true; + return continueProcessing; } @Override public void afterCompletion(NativeWebRequest request, DeferredResult deferredResult) { - synchronized (DeferredResult.this) { - expired = true; - } + expired = true; if (completionCallback != null) { completionCallback.run(); }