jetty.project icon indicating copy to clipboard operation
jetty.project copied to clipboard

Fix buffer leaks in FCGI and H3 `HttpClientIdleTimeoutTest`

Open lorban opened this issue 2 years ago • 7 comments

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 https://github.com/eclipse/jetty.project/issues/10226

lorban avatar Aug 29 '23 14:08 lorban

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 (https://github.com/eclipse/jetty.project/pull/10325#issuecomment-1695235810), and we agreed that, as it stands, the leak tracker has little value outside the scope of our tests.

lorban avatar Aug 29 '23 15:08 lorban

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.

lorban avatar Aug 29 '23 15:08 lorban

On hold until https://github.com/eclipse/jetty.project/issues/10277 is sorted.

lorban avatar Sep 13 '23 16:09 lorban

@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 avatar Dec 15 '23 11:12 lorban

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

gregw avatar Jan 25 '24 07:01 gregw

@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 ?

janbartel avatar Jan 25 '24 07:01 janbartel

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

olamy avatar Jan 26 '24 10:01 olamy

@sbordet nudge

lorban avatar Feb 20 '24 13:02 lorban