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
headersTimeout and connectTimeout together create a resource leak #59
Comments
BTW, do you think this is a correct diagram?
Because it looks like (according to this issue https://bugs.openjdk.org/browse/JDK-8258397) requestTimeout actually canceled when headers are received. |
Hi @rymsha. Does the leak happen when
Hmm, I actually always expected that request timeouts cover receiving the response body. If that's not the case, then there's no functional difference between request & headers timeouts. That'd make the diagram look like this:
I can verify this is actually the case ( |
Only when used together. Comment out either connectTimeout or headersTimeout (or both 😀) and the leak disappears. |
After investing this further, I believe there's no memory leak. However, headers timeout tasks may keep references to HTTP client resources until the timeout is evaluated (regardless of whether the response completed or not). This is because it uses the system-wide scheduler maintained by var scheduler = new ScheduledThreadPoolExecutor(1);
scheduler.setRemoveOnCancelPolicy(true); // Lose references to timeout tasks when they're cancelled.
var client = Methanol.newBuilder()
.headersTimeout(Duration.ofSeconds(25), scheduler)
.build(); BTW the screenshot you've provided shows the references path: |
When
connectTimeout
headersTimeout
are used togetherHttpClient is never garbage collected:
I guess a simple workaround is not to use
connectTimeout
whenheadersTimeout
is set. But I still curious if it can be improved.Tested on JDK 11 latest.
The text was updated successfully, but these errors were encountered: