New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #11679 - Jetty 12.0.8 seems to leak connection when it encounters earlyEOF. #11719
Fixes #11679 - Jetty 12.0.8 seems to leak connection when it encounters earlyEOF. #11719
Conversation
…rs earlyEOF. Changed HttpConnection.RequestHandler.earlyEOF() to produce EofException instead of BadMessageException, as it is more appropriate. Changed handling of HttpChannelState.onFailure() to not fail the write side unless there is a pending write callback. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are plenty of tests expecting a response status 400 but now getting 500 instead.
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Show resolved
Hide resolved
@@ -1057,7 +1057,7 @@ public void earlyEOF() | |||
HttpStreamOverHTTP1 stream = _stream.get(); | |||
if (stream != null) | |||
{ | |||
BadMessageException bad = new BadMessageException("Early EOF"); | |||
EofException bad = new EofException("Early EOF"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that this sends a 500. I accept that perhaps the request is not bad and it is a connection problem, but it is certainly not a server problem as the 500 suggests.
How about:
EofException bad = new EofException("Early EOF"); | |
BadMessageException bad = new BadMessageException(null, new EofException("Early EOF")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be a type of EOF exception because that's what it is.
I have introduced an HttpEofException
so it is converted to a 400, but it is-a IOException
, and it's quiet because it's a client close, so won't clutter the logs.
…tion. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still fails some related tests...
…tation. Now failures only impact pending writes, so that it would be possible to write an HTTP error response. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, besides one niggle.
@@ -1057,7 +1057,7 @@ public void earlyEOF() | |||
HttpStreamOverHTTP1 stream = _stream.get(); | |||
if (stream != null) | |||
{ | |||
BadMessageException bad = new BadMessageException("Early EOF"); | |||
HttpEofException bad = new HttpEofException("Early EOF"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either hardcode the "Early EOF"
message in the exception's getReason()
method, or return the message passed to the constructor, but let's not mix both.
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @lorban comment... other than that LGTM.
Changed HttpConnection.RequestHandler.earlyEOF() to produce EofException instead of BadMessageException, as it is more appropriate.
Changed handling of HttpChannelState.onFailure() to not fail the write side unless there is a pending write callback.