Issue #8211 - fix ISE from HttpChannelState if failure during process #8215
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
| { | ||
| try (AutoLock ignored = _lock.lock()) | ||
| { | ||
| if (request._httpChannel != null && !request._httpChannel._completed) | ||
| request._callback.failed(failure); | ||
| else if (LOG.isDebugEnabled()) | ||
| LOG.debug("Process failed", failure); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potentially it would be neater to put this logic into a method:
| { | |
| try (AutoLock ignored = _lock.lock()) | |
| { | |
| if (request._httpChannel != null && !request._httpChannel._completed) | |
| request._callback.failed(failure); | |
| else if (LOG.isDebugEnabled()) | |
| LOG.debug("Process failed", failure); | |
| } | |
| } | |
| request._callback.ensureCompleted(failure); |
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Lachlan Roberts <[email protected]>
gregw
dismissed
their stale review
LGTM, but as a coauther will defer to others to approve
…8211-channelAlreadyComplete
…8211-channelAlreadyComplete
…8211-channelAlreadyComplete
Signed-off-by: Lachlan Roberts <[email protected]>
…tions Signed-off-by: Lachlan Roberts <[email protected]>
…x-8211-channelAlreadyComplete'.
…ready HttpChannelState._completed. Now all the logic for completion is in lockedOnComplete(). Avoid throwing in ChannelCallback.succeeded() if HttpChannelState._error != null. This is necessary because HttpChannelState._error may be set asynchronously by some event such as HTTP/2 reset streams or idle timeouts, but if there is a thread dispatched to the application the asynchronous event will not fail the callback, as the failure may be noticed by the application (e.g. via a read() call). Fixed TrailersTest.testHandlerRequestTrailers() by avoid reading again after having read the trailers. Signed-off-by: Simone Bordet <[email protected]>
| @Test | ||
| public void testForgivingFailedClientEnforcingStreamIdleTimeout() throws Exception | ||
| { | ||
| final int idleTimeout = 1000; |
There was a problem hiding this comment.
No need for final, here and below.
| @@ -1317,6 +1319,10 @@ public void succeeded() | |||
| boolean completeStream; | |||
| try (AutoLock ignored = _request._lock.lock()) | |||
| { | |||
| if (_completed) | |||
| return; | |||
| _completed = true; | |||
There was a problem hiding this comment.
I would not do this check here.
The call below lockedOnComplete() is the one responsible for setting _completed to true, and throwing if it's already set.
I would: A) make lockedOnComplete() return a boolean, and if it is false return; and B) remove the code in lockedOnComplete() that throws, which is now never executed.
| @@ -1377,6 +1383,10 @@ public void failed(Throwable failure) | |||
| boolean completeStream; | |||
| try (AutoLock ignored = _request._lock.lock()) | |||
| { | |||
| if (_completed) | |||
| return; | |||
| _completed = true; | |||
| if (failure != null) | ||
| request._callback.failed(failure); | ||
| _request._callback.ensureCompleted(failure); |
There was a problem hiding this comment.
I am not sure I understand this. With the current modifications, the original code calling failed() is already doing the check on _completed, so this method does nothing different, and certainly does not "ensure" and it only fails the callback, while the name "ensureCompleted" seems to hint that it either succeeds or fails it.
I would restore the old code.
|
@lachlan-roberts I have pushed changes that align to my comments, so I'll merge this PR once build passes. |
If a timeout or stream reset occurs during process, the
HttpChannelStatewill be failed, this causescallback.succeeded()to throw ISE. This ISE is then caught inHandlerInvokerand the callback is failed, and because the callback was failed twice we get thejava.lang.IllegalStateException: already completedseen in #8211.This PR fixes this by
HandlerInvokerifHttpChannelhas not been completed.callback.failed()if the callback is failed with same cause as why theHttpChannelwas failed.