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 buffer leaks in FCGI and H3 HttpClientIdleTimeoutTest #10432

Merged
merged 22 commits into from
Feb 23, 2024
Merged

Fix buffer leaks in FCGI and H3 HttpClientIdleTimeoutTest #10432

merged 22 commits into from
Feb 23, 2024

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Aug 29, 2023 •
edited
Loading

testClientIdleTimeout and testRequestIdleTimeout leak a buffer because the handler's callback is never completed and they don't give the server a chance to reclaim its resources after idle timeout.

Note: the leak was only detected in FCGI because the client was not uploading any content. Adding some uploaded content uncovered an equivalent leak in H3.

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 review from gregw and sbordet August 29, 2023 14:13
@lorban lorban self-assigned this Aug 29, 2023
joakime
joakime previously approved these changes Aug 29, 2023
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

So this leak was entirely contained within the test case.
Interesting.
Wonder if we should document how to turn on Leak tracking in our developer guide?

@lorban
Copy link
Contributor Author

lorban commented Aug 29, 2023

So this leak was entirely contained within the test case. Interesting. Wonder if we should document how to turn on Leak tracking in our developer guide?

I'm open to the idea, but the leak tracking buffer pool currently is rough on the edges, so I'm not keen on exposing it to end users as it currently is. @gregw had a similar suggestion here (#10325 (comment)), and we agreed that, as it stands, the leak tracker has little value outside the scope of our tests.

@lorban
Copy link
Contributor Author

lorban commented Aug 29, 2023

This requires more investigation as other protocols manage to release their buffers despite the callback not being completed. We, at the very least, need to understand why not FCGI.

gregw
gregw previously approved these changes Sep 8, 2023
View reviewed changes
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.

Can we fix FCGI instead of just the test?

@lorban
Copy link
Contributor Author

lorban commented Sep 13, 2023

On hold until #10277 is sorted.

@lorban lorban changed the title Fix buffer leaks in FCGI HttpClientIdleTimeoutTest [Blocked by #10277] Fix buffer leaks in FCGI HttpClientIdleTimeoutTest Nov 21, 2023
@joakime joakime mentioned this pull request Nov 29, 2023
Closed
2 tasks
@joakime joakime linked an issue Nov 29, 2023 that may be closed by this pull request
Review read failures impacting writes #10277
Closed
2 tasks
@lorban
- fix HttpClientIdleTimeoutTest to wait for server's idle timeout bef…
a424ae3 
…ore checking for leaks

- improve HttpClientIdleTimeoutTest by making it upload some content
- fix FCGI server leak caused by idle timeout
- fix H3 server leak caused by idle timeout

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban dismissed stale reviews from gregw and joakime via a424ae3 December 15, 2023 11:52
@lorban lorban force-pushed the fix/jetty-12-10226-fcgi-HttpClientIdleTimeoutTest-leaks branch from 1855c8d to a424ae3 Compare December 15, 2023 11:52
@lorban lorban changed the title [Blocked by #10277] Fix buffer leaks in FCGI HttpClientIdleTimeoutTest [Blocked by #10277] Fix buffer leaks in FCGI and H3 HttpClientIdleTimeoutTest Dec 15, 2023
@lorban lorban requested review from sbordet, gregw and joakime December 15, 2023 11:55
@lorban
Copy link
Contributor Author

lorban commented Dec 15, 2023

@sbordet @gregw @joakime I've reworked these tests by using Request.addFailureListener() to detect when the server had a chance to reclaim its resources, identified and fixed the leak in FCGI and slightly improved the tests such as a similar leak became apparent in H3 which I also fixed.

This PR is finally ready for review.

@lorban lorban changed the title [Blocked by #10277] Fix buffer leaks in FCGI and H3 HttpClientIdleTimeoutTest Fix buffer leaks in FCGI and H3 HttpClientIdleTimeoutTest Dec 15, 2023
@lorban
handle review comments
27da0b9 
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
1659401 
…10226-fcgi-HttpClientIdleTimeoutTest-leaks
@lorban lorban requested a review from sbordet December 18, 2023 08:40
@gregw
Copy link
Contributor

gregw commented Jan 25, 2024

@lorban the OSGI CI failures look like hard failures. @janbartel any ideas to help out?

@janbartel
Copy link
Contributor

@gregw @lorban there is a new osgi test that has been added which is failing in this branch. It seems like the new test class is there, but the new xml file that should be present in the jar is not there? Could this be a build cache issue? @olamy ?

lorban added 6 commits January 26, 2024 18:37
@lorban @olamy
- fix HttpClientIdleTimeoutTest to wait for server's idle timeout bef…
7c394d9 
…ore checking for leaks

- improve HttpClientIdleTimeoutTest by making it upload some content
- fix FCGI server leak caused by idle timeout
- fix H3 server leak caused by idle timeout

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban @olamy
handle review comments
8cd3f95 
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban @olamy
fix missing acquire
0c4ad22 
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban @olamy
release the network buffer when the parser commands to stop and it is…
82cda98 
… empty

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban @olamy
release the network buffer when the parser commands to stop and it is…
7e9ff18 
… empty

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban @olamy
fix over-releases and add comment
4dbd35a 
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@olamy olamy force-pushed the fix/jetty-12-10226-fcgi-HttpClientIdleTimeoutTest-leaks branch from fa05eda to 4dbd35a Compare January 26, 2024 08:37
@olamy
Copy link
Member

olamy commented Jan 26, 2024 •
edited
Loading

I can't see any more issues after rebasing from jetty-12 branch

lorban added 3 commits January 30, 2024 10:24
@lorban
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
5f0891a 
…10226-fcgi-HttpClientIdleTimeoutTest-leaks
@lorban
Merge branch 'fix/jetty-12-10226-fcgi-HttpClientIdleTimeoutTest-leaks…
57fbdee 
…' of github.com:jetty/jetty.project into fix/jetty-12-10226-fcgi-HttpClientIdleTimeoutTest-leaks
@lorban
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
b08a292 
…10226-fcgi-HttpClientIdleTimeoutTest-leaks
@lorban
Copy link
Contributor Author

lorban commented Feb 20, 2024

@sbordet nudge

gregw
gregw requested changes Feb 22, 2024
View reviewed changes
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.

a few niggles

...y-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/ServerFCGIConnection.java Outdated Show resolved Hide resolved
...y-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/ServerFCGIConnection.java Outdated Show resolved Hide resolved
...ty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/HTTP3StreamConnection.java Outdated Show resolved Hide resolved
@lorban
handle review comments
67b5c5d 
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
81f51a5 
…10226-fcgi-HttpClientIdleTimeoutTest-leaks
@lorban lorban requested a review from gregw February 22, 2024 14:25
@sbordet
Using the input buffer size for the network buffer.
c783fcb 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet
sbordet previously approved these changes Feb 22, 2024
View reviewed changes
@lorban
handle review comments
2bd8896 
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban requested a review from sbordet February 22, 2024 14:54
@lorban lorban merged commit efc2785 into jetty-12.0.x Feb 23, 2024
8 checks passed
@lorban lorban deleted the fix/jetty-12-10226-fcgi-HttpClientIdleTimeoutTest-leaks branch February 23, 2024 12:37
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

@gregw gregw gregw approved these changes

@joakime joakime Awaiting requested review from joakime

Assignees

@lorban lorban

Labels
Bug For general bugs on Jetty side
Projects
No open projects
Jetty 12.0.7 - FROZEN
Status: ✅ Done
Milestone
12.0.x
Development

Successfully merging this pull request may close these issues.

Review read failures impacting writes
6 participants
@lorban @gregw @janbartel @olamy @joakime @sbordet