HTTPCLIENT-2388: fix builder default evictor sleepTime

[sanghwa-min]

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.

[ok2c]

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

[sanghwa-min]

@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. With this perspective, this might be preferable because, in the current implementation, a connection can survive for nearly 40s. For example, if the eviction check runs every 20s, a connection that has been idle for 19.9s survives. It then remains open until the next check 20s later, ultimately violating the user's explicit configuration. The primary goal of this change is to strictly enforce the configured 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.

[arturobernalg]

Halving both sleepTime and maxIdleTime changes semantics: the threshold “evict idle > T” becomes “evict well before T,” violating user intent.

[sanghwa-min]

@arturobernalg @ok2c
You are right, I see your point.
Thank you for your feedbacks. Closing this PR.

[ok2c]

@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 interval.

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

[sanghwa-min]

@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 even 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 or different divisor would be better, please let me know.

[ok2c]

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

[sanghwa-min]

@ok2c
I agree. Updated sleepTime goes minimum to TimeValue of 1 second.

[ok2c]

@sanghwa-min Let's make static ONE_SECONDvariable for TimeValue.ofSeconds(1L) . Looks good otherwise.

[sanghwa-min]

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

[ok2c]

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

[sanghwa-min]

@ok2c Understood. I updated the commit, thank you.