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.

Sign up for GitHub

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

Jump to bottom

Fix server HTTP buffer leak in consumeAvailable() #10428

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Fix server HTTP buffer leak in consumeAvailable() #10428

merged 4 commits into from
Sep 11, 2023

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Aug 29, 2023 •
edited
Loading

When consumeAvailable() is called, the connection may hold onto a buffer that must be released.

See #10226

@lorban lorban added the Bug For general bugs on Jetty side label Aug 29, 2023
@lorban lorban added this to the 12.0.x milestone Aug 29, 2023
@lorban lorban requested a review from sbordet August 29, 2023 09:19
@lorban lorban self-assigned this Aug 29, 2023
@lorban lorban requested a review from sbordet August 29, 2023 13:14
@lorban
Copy link
Contributor Author

lorban commented Aug 29, 2023 •
edited
Loading

This requires more investigation as HttpStream.consumeAvailable() should be able to release any pending buffers. And #9169 might be related.

@lorban lorban force-pushed the fix/jetty-12-10226-fix-Http-consumeAvailable-leak branch from 4f45bdd to 58a5499 Compare August 30, 2023 07:56
@lorban lorban requested a review from joakime August 30, 2023 07:56
@lorban
Copy link
Contributor Author

lorban commented Aug 30, 2023

HttpStream.consumeAvailable() is designed such that it does not necessarily release all buffers but returns non-null when such a case occurs. This is the trigger condition to indicate that the buffer can be released.

sbordet
sbordet requested changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to find the root cause of why the buffers are leaked.

@lorban
#10226 handle review comments
a643194 
Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban requested a review from sbordet September 8, 2023 14:20
@lorban lorban merged commit 022c99c into jetty-12.0.x Sep 11, 2023
2 checks passed
@lorban lorban deleted the fix/jetty-12-10226-fix-Http-consumeAvailable-leak branch September 11, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@joakime joakime Awaiting requested review from joakime

Assignees

@lorban lorban

Labels
Bug For general bugs on Jetty side
Projects
None yet
Milestone
12.0.x
Development

Successfully merging this pull request may close these issues.
3 participants
@lorban @joakime @sbordet