You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "sanghwa-min (via GitHub)" <gi...@apache.org> on 2025/08/26 07:17:24 UTC
[PR] HTTPCLIENT-2388: fix builder default evictor sleepTime and maxIdleTime to half [httpcomponents-client]
sanghwa-min opened a new pull request, #714:
URL: https://github.com/apache/httpcomponents-client/pull/714
[HTTPCLIENT-2388](https://issues.apache.org/jira/browse/HTTPCLIENT-2388)
Users of the HttpClientBuilder.evictIdleConnections(maxIdleTime) method likely expect the maximum idle time to be maxIdleTime, not up to `2 * maxIdleTime`.
This PR proposes setting the default sleepTime and maxIdleTime for the internal IdleConnectionEvictor to maxIdleTime / 2.
For example, with a maxIdleTime of 20s, the effective eviction window changes from 20-40s to 10-20s. This ensures the idle time does not exceed the configured value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#discussion_r2306944899
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java:
##########
@@ -1094,8 +1095,10 @@ public CloseableHttpAsyncClient build() {
}
if (evictExpiredConnections || evictIdleConnections) {
if (connManagerCopy instanceof ConnPoolControl) {
+ TimeValue sleepTime = maxIdleTime.divide(10, TimeUnit.NANOSECONDS);
+ sleepTime = sleepTime.compareTo(TimeValue.ofSeconds(1L)) < 0 ? TimeValue.ofSeconds(1L) : sleepTime;
Review Comment:
@sanghwa-min For now just add a private static variables to the builder classes. At a later point we can add `ONE_SECOND` static variable to the `TimeValue` in core.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime and maxIdleTime to half [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#issuecomment-3227264370
@sanghwa-min Now help me understand. The user configures the client to evict connections idle longer than 20 seconds. With the proposed changes the IdleConnectionEvictor will wake up in 10 seconds and drop connection that have been idle longer than 10 seconds. The sweep will be completed in 10 seconds plus a small overhead involved in iterating over pool entries. The net result is that there will be no connections idle longer than 10 seconds plus a few milliseconds.
How does this make sense with the setting of 20 seconds?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c merged PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime and maxIdleTime to half [httpcomponents-client]
Posted by "sanghwa-min (via GitHub)" <gi...@apache.org>.
sanghwa-min commented on PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#issuecomment-3227626914
@ok2c cc. @arturobernalg
The default sleepTime is now set to `maxIdleTime / 10`, while maxIdleTime remains untouched. This brings the maximum survival boundary for an idle connection to `1.1 * maxIdleTime` plus the eviction execution time.
Choosing the right divisor is a trade-off between guaranteeing the idle timeout and CPU usage—ideally it would be near zero, but that's not feasible. I'm not sure whether this value should be a fixed default or a new configurable field `sleepTime`, since users can instantiate `IdleConnectionEvictor` directly for full control.
If you think making the divisor adjustable would be better, please let me know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#discussion_r2304671943
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java:
##########
@@ -1094,8 +1095,10 @@ public CloseableHttpAsyncClient build() {
}
if (evictExpiredConnections || evictIdleConnections) {
if (connManagerCopy instanceof ConnPoolControl) {
+ TimeValue sleepTime = maxIdleTime.divide(10, TimeUnit.NANOSECONDS);
+ sleepTime = sleepTime.compareTo(TimeValue.ofSeconds(1L)) < 0 ? TimeValue.ofSeconds(1L) : sleepTime;
Review Comment:
@sanghwa-min Let's make static `ONE_SECOND`variable for `TimeValue.ofSeconds(1L)` . Looks good otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime [httpcomponents-client]
Posted by "sanghwa-min (via GitHub)" <gi...@apache.org>.
sanghwa-min commented on code in PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#discussion_r2306157811
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java:
##########
@@ -1094,8 +1095,10 @@ public CloseableHttpAsyncClient build() {
}
if (evictExpiredConnections || evictIdleConnections) {
if (connManagerCopy instanceof ConnPoolControl) {
+ TimeValue sleepTime = maxIdleTime.divide(10, TimeUnit.NANOSECONDS);
+ sleepTime = sleepTime.compareTo(TimeValue.ofSeconds(1L)) < 0 ? TimeValue.ofSeconds(1L) : sleepTime;
Review Comment:
@ok2c
Yes, that's a good point. Adding a constant would improve readability.
I actually considered this in a previous commit, along with a new `TimeValue.max(TimeValue other)` method. However, I implemented the logic in the `Builder` instead, after realizing that `TimeValue` belongs to the separate `httpcomponent-core` project.
To clarify, are you suggesting that the `ONE_SECOND` constant should be a `public static final` field within each `Builder` class?
If you think modifying the `TimeValue` class is the more appropriate approach, I will open a new PR for the `httpcomponent-core` project.
Thank you for your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime [httpcomponents-client]
Posted by "sanghwa-min (via GitHub)" <gi...@apache.org>.
sanghwa-min commented on code in PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#discussion_r2306157811
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java:
##########
@@ -1094,8 +1095,10 @@ public CloseableHttpAsyncClient build() {
}
if (evictExpiredConnections || evictIdleConnections) {
if (connManagerCopy instanceof ConnPoolControl) {
+ TimeValue sleepTime = maxIdleTime.divide(10, TimeUnit.NANOSECONDS);
+ sleepTime = sleepTime.compareTo(TimeValue.ofSeconds(1L)) < 0 ? TimeValue.ofSeconds(1L) : sleepTime;
Review Comment:
@ok2c
Yes, that's a good point. Adding a constant would be great.
I actually considered this in a previous commit, along with a new `TimeValue.max(TimeValue other)` method. However, I implemented the logic in the `Builder` instead, after realizing that `TimeValue` belongs to the separate `httpcomponent-core` project.
To clarify, are you suggesting that the `ONE_SECOND` constant should be a `public static final` field within each `Builder` class?
If you think modifying the `TimeValue` class is the more appropriate approach, I will open a new PR for the `httpcomponent-core` project.
Thank you for your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#issuecomment-3227705393
@sanghwa-min There is a way to start `IdleConnectionEvictor` with custom settings or even implement a custom one. No need for more parameters.
What would ask you to do is to ensure that `sleepTime` is positive and probably is not lesser than a second. I see no point in sweeping connections more often than once a second.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime and maxIdleTime to half [httpcomponents-client]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#issuecomment-3227380771
IMO Halving both `sleepTime` and `maxIdleTime` changes semantics: the threshold “evict idle > T” becomes “evict well before T,” violating user intent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime and maxIdleTime to half [httpcomponents-client]
Posted by "sanghwa-min (via GitHub)" <gi...@apache.org>.
sanghwa-min commented on PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#issuecomment-3227348839
@ok2c
Hi,
You are exactly right.
However, my perspective is that when users employ the evictIdleConnections method, they do so based on the method name and its Javadoc, with the expectation of guaranteeing a maximum idle time for connections in the pool. This is often to proactively perform a client-side close, typically because they are aware of the server's idle timeout settings.
Well, it becomes a matter of choice. With a 20s setting, some connections that have been idle for only 10s will indeed be evicted. This is a trade-off to ensure that the maximum idle time remains at 20s + execution time. This might be preferable to the current implementation where connections can potentially survive for up to 40s, which could violates the user's explicit configuration. The primary goal is to restrict maxIdleTime.
However, if you feel I'm making too broad of an assumption about user expectations, please let me know and I will close this PR.
Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime [httpcomponents-client]
Posted by "sanghwa-min (via GitHub)" <gi...@apache.org>.
sanghwa-min commented on PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#issuecomment-3227908558
@ok2c
I agree. Updated sleepTime goes minimum to TimeValue of 1 second.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime and maxIdleTime to half [httpcomponents-client]
Posted by "sanghwa-min (via GitHub)" <gi...@apache.org>.
sanghwa-min commented on PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#issuecomment-3227410210
@arturobernalg @ok2c
You are right, I see your point.
Thank you for your feedbacks. Closing this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime [httpcomponents-client]
Posted by "sanghwa-min (via GitHub)" <gi...@apache.org>.
sanghwa-min commented on code in PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#discussion_r2306157811
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java:
##########
@@ -1094,8 +1095,10 @@ public CloseableHttpAsyncClient build() {
}
if (evictExpiredConnections || evictIdleConnections) {
if (connManagerCopy instanceof ConnPoolControl) {
+ TimeValue sleepTime = maxIdleTime.divide(10, TimeUnit.NANOSECONDS);
+ sleepTime = sleepTime.compareTo(TimeValue.ofSeconds(1L)) < 0 ? TimeValue.ofSeconds(1L) : sleepTime;
Review Comment:
@ok2c
Yes, that's a good point. Adding a constant would be great.
I actually considered this in a previous commit, along with a new `TimeValue.max(TimeValue other)` method. However, I implemented the logic in the `Builder` instead, after realizing that `TimeValue` belongs to the separate `httpcomponent-core` project.
To clarify, are you suggesting that the `ONE_SECOND` constant should be a `static final` field within each `Builder` class?
If you think modifying the `TimeValue` class is the more appropriate approach, I will open a new PR for the `httpcomponent-core` project.
Thank you for your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#discussion_r2306944899
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java:
##########
@@ -1094,8 +1095,10 @@ public CloseableHttpAsyncClient build() {
}
if (evictExpiredConnections || evictIdleConnections) {
if (connManagerCopy instanceof ConnPoolControl) {
+ TimeValue sleepTime = maxIdleTime.divide(10, TimeUnit.NANOSECONDS);
+ sleepTime = sleepTime.compareTo(TimeValue.ofSeconds(1L)) < 0 ? TimeValue.ofSeconds(1L) : sleepTime;
Review Comment:
@sanghwa-min For now just add a private static variable to the builder classes. At a later point we can add `ONE_SECOND` static variable to the `TimeValue` in core.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime and maxIdleTime to half [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#issuecomment-3227432282
@arturobernalg I realized this is about the worst case scenario. It can happen that a connection survives the first sweep at n interval and gets dropped with the next sweep at nearly 2 * n times.
@sanghwa-min Do not give up too soon. I think you are fundamentally right. Can make the evictor wake up more often but keep the `maxIdleTime` unchanged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime and maxIdleTime to half [httpcomponents-client]
Posted by "sanghwa-min (via GitHub)" <gi...@apache.org>.
sanghwa-min closed pull request #714: HTTPCLIENT-2388: fix builder default evictor sleepTime and maxIdleTime to half
URL: https://github.com/apache/httpcomponents-client/pull/714
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] HTTPCLIENT-2388: fix builder default evictor sleepTime [httpcomponents-client]
Posted by "sanghwa-min (via GitHub)" <gi...@apache.org>.
sanghwa-min commented on code in PR #714:
URL: https://github.com/apache/httpcomponents-client/pull/714#discussion_r2306960178
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java:
##########
@@ -1094,8 +1095,10 @@ public CloseableHttpAsyncClient build() {
}
if (evictExpiredConnections || evictIdleConnections) {
if (connManagerCopy instanceof ConnPoolControl) {
+ TimeValue sleepTime = maxIdleTime.divide(10, TimeUnit.NANOSECONDS);
+ sleepTime = sleepTime.compareTo(TimeValue.ofSeconds(1L)) < 0 ? TimeValue.ofSeconds(1L) : sleepTime;
Review Comment:
@ok2c Understood. I updated the commit, thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org