Skip to content
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

Merged
merged 7 commits into from May 3, 2024

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Apr 29, 2024

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.

…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>
@sbordet sbordet requested review from lorban and gregw April 29, 2024 16:26
Copy link
Contributor

@lorban lorban left a 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>
@@ -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");
Copy link
Contributor

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:

Suggested change
EofException bad = new EofException("Early EOF");
BadMessageException bad = new BadMessageException(null, new EofException("Early EOF"));

Copy link
Contributor Author

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>
Copy link
Contributor

@lorban lorban left a 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>
@sbordet sbordet requested a review from lorban May 2, 2024 17:19
lorban
lorban previously approved these changes May 3, 2024
Copy link
Contributor

@lorban lorban left a 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");
Copy link
Contributor

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>
gregw
gregw previously approved these changes May 3, 2024
Copy link
Contributor

@gregw gregw left a 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.

@lorban lorban linked an issue May 3, 2024 that may be closed by this pull request
@joakime joakime merged commit 724273b into jetty-12.0.x May 3, 2024
2 of 4 checks passed
@joakime joakime deleted the fix/jetty-12/11679/early-eof-jersey-leak branch May 3, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Jetty 12.0.8 seems to leak connection when it encounters earlyEOF
4 participants