Issue #5605 - Async Request Timeout results in Blocked Thread #5923
Conversation
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
There was a problem hiding this comment.
So in general I think the approach is OK:
- we are failing HttpInput on sendError and termination so any random thread that tries to read after that point will have a problem. We are also cancelling any associated fillInterest at the same time.
- we have replaced calls to plain
fillInterestwith versions that can cope with being register-bombed by some random thread - by either turning into a noop or cancelling the bomber.
My concern is that it may not be universal, but then it wasn't before. So if we can at least make it low cost and understandable, we are improving code.
| @@ -366,6 +366,8 @@ public boolean handle() | |||
| { | |||
| case TERMINATED: | |||
| onCompleted(); | |||
| _request.getHttpInput().failed(new IOException("terminated")); | |||
There was a problem hiding this comment.
Ideally this exception should only be created if it is needed.
| @@ -366,6 +366,8 @@ public boolean handle() | |||
| { | |||
| case TERMINATED: | |||
| onCompleted(); | |||
| _request.getHttpInput().failed(new IOException("terminated")); | |||
| getEndPoint().cancelFillInterested(); | |||
There was a problem hiding this comment.
If we do create an exception for the step above, it should be passed into this cancel so we don't create another one. Or maybe the HttpInput does this cancel for us?
| _request.getHttpInput().failed(new IOException("cancellation")); | ||
| getEndPoint().cancelFillInterested(); |
There was a problem hiding this comment.
If there is an exception associated with the sendError, it should be used for the fail and the cancel.
If a new exception is needed, it should be the same exception for the failed and the cancel
perhaps httpInput.failed can do the the cancel
| @@ -278,7 +278,7 @@ else if (filled == -1 && getEndPoint().isOutputShutdown()) | |||
| } | |||
| else if (filled == 0) | |||
| { | |||
| fillInterested(); | |||
| tryFillInterested(); | |||
There was a problem hiding this comment.
We need to review all other calls to fillInterested as any of them can be register-bombed by some random thread doing a read
Signed-off-by: Ludovic Orban <[email protected]>
|
Don't forget to to do the metadata on this PR: link the issue, add labels, add to a project, etc ... |
joakime
changed the title
Signed-off-by: Ludovic Orban <[email protected]>
| getEndPoint().cancelFillInterested(); | ||
| if (_request.getHttpInput().isError()) | ||
| { | ||
| _request.getHttpInput().failed(new IOException("terminated")); |
There was a problem hiding this comment.
If we are already in error, then why do we need to fail again? Any caller will not block in read and get an exception either way.
There was a problem hiding this comment.
If we blindly fail the input upon termination, any content already parsed but not yet consumed by the servlet will be discarded and make the servlet throw an exception while there was no error. If we do not fail again, then there's still the risk of a thread still being stuck in a blocking read after the async timeout.
This extra hack seems to fix a good chunk of the tests that failed during the previous build, it just adds to the cleanup that will be needed to get this PR to a reasonable state.
There was a problem hiding this comment.
I don't doubt we need to be smart here and not break chained requests. I was thinking more that if we are at EOF then we do nothing.
What I don't understand is how replacing and error with another error is helping? Is it just the cancel on the next line?
I think we need to understand why this works.
joakime
deleted the
jetty-9.4.x-5605-async-request-timeout-blocked-thread
branch
Fixes #5605