[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #437: Improve AIMDBackoffManager with atomic references, thread-safety, and parameter checks

["ok2c (via GitHub)" <gi...@apache.org>]

ok2c commented on code in PR #437:
URL: https://github.com/apache/httpcomponents-client/pull/437#discussion_r1181067629


##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/AIMDBackoffManager.java:
##########
@@ -56,79 +62,134 @@
  *
  * @since 4.2
  */
-@Experimental
+@Contract(threading = ThreadingBehavior.SAFE)
 public class AIMDBackoffManager implements BackoffManager {
 
+    private static final Logger LOG = LoggerFactory.getLogger(AIMDBackoffManager.class);
+
+
+    /**
+     * Represents the connection pool control object for managing
+     * the maximum number of connections allowed per HTTP route.
+     */
     private final ConnPoolControl<HttpRoute> connPerRoute;
-    private final Clock clock;
-    private final Map<HttpRoute, Long> lastRouteProbes;
-    private final Map<HttpRoute, Long> lastRouteBackoffs;
-    private TimeValue coolDown = TimeValue.ofSeconds(5L);
-    private double backoffFactor = 0.5;
-    private int cap = 2; // Per RFC 2616 sec 8.1.4
 
     /**
-     * Creates an {@code AIMDBackoffManager} to manage
-     * per-host connection pool sizes represented by the
-     * given {@link ConnPoolControl}.
-     * @param connPerRoute per-host routing maximums to
-     *   be managed
+     * A map that keeps track of the last time a successful
+     * connection was made for each HTTP route. Used for
+     * adjusting the pool size when probing.
      */
-    public AIMDBackoffManager(final ConnPoolControl<HttpRoute> connPerRoute) {
-        this(connPerRoute, new SystemClock());
-    }
+    private final Map<HttpRoute, Instant> lastRouteProbes;
+
+    /**
+     * A map that keeps track of the last time a connection
+     * failure occurred for each HTTP route. Used for
+     * adjusting the pool size when backing off.
+     */
+    private final Map<HttpRoute, Instant> lastRouteBackoffs;
 
-    AIMDBackoffManager(final ConnPoolControl<HttpRoute> connPerRoute, final Clock clock) {
-        this.clock = clock;
-        this.connPerRoute = connPerRoute;
-        this.lastRouteProbes = new HashMap<>();
-        this.lastRouteBackoffs = new HashMap<>();
+    /**
+     * The cool-down time between adjustments in pool sizes for a given host.
+     * This time value allows enough time for the adjustments to take effect.
+     * Defaults to 5 seconds.
+     */
+    private final AtomicReference<TimeValue> coolDown = new AtomicReference<>(TimeValue.ofSeconds(5L));
+
+    /**
+     * The factor to use when backing off; the new per-host limit will be
+     * roughly the current max times this factor. {@code Math.floor} is
+     * applied in the case of non-integer outcomes to ensure we actually
+     * decrease the pool size. Pool sizes are never decreased below 1, however.
+     * Defaults to 0.5.
+     */
+    private final AtomicReference<Double> backoffFactor = new AtomicReference<>(0.5);
+
+    /**
+     * The absolute maximum per-host connection pool size to probe up to.
+     * Defaults to 2 (the default per-host max as per RFC 2616 section 8.1.4).
+     */
+    private final AtomicInteger cap = new AtomicInteger(2); // Per RFC 2616 sec 8.1.4
+
+
+    /**
+     * Constructs an {@code AIMDBackoffManager} with the specified
+     * {@link ConnPoolControl} and {@link Clock}.
+     * <p>
+     * This constructor is primarily used for testing purposes, allowing the
+     * injection of a custom {@link Clock} implementation.
+     *
+     * @param connPerRoute the {@link ConnPoolControl} that manages
+     *                     per-host routing maximums
+     */
+    public AIMDBackoffManager(final ConnPoolControl<HttpRoute> connPerRoute) {
+        this.connPerRoute = Args.notNull(connPerRoute, "Connection pool control");
+        this.lastRouteProbes = new ConcurrentHashMap<>();
+        this.lastRouteBackoffs = new ConcurrentHashMap<>();
     }
 
     @Override
     public void backOff(final HttpRoute route) {
-        synchronized(connPerRoute) {
-            final int curr = connPerRoute.getMaxPerRoute(route);
-            final Long lastUpdate = getLastUpdate(lastRouteBackoffs, route);
-            final long now = clock.getCurrentTime();
-            if (now - lastUpdate < coolDown.toMilliseconds()) {
-                return;
+        final int curr = connPerRoute.getMaxPerRoute(route);
+        final Instant now = Instant.now();
+
+        lastRouteBackoffs.compute(route, (r, lastUpdate) -> {
+            if (lastUpdate == null || now.isAfter(lastUpdate.plus(Duration.ofMillis(coolDown.get().toMilliseconds())))) {
+                if (LOG.isDebugEnabled()) {
+                    LOG.debug("Backoff applied for route: {}, new max connections: {}", route, connPerRoute.getMaxPerRoute(route));
+                }
+                connPerRoute.setMaxPerRoute(route, getBackedOffPoolSize(curr));
+                return now;
             }
-            connPerRoute.setMaxPerRoute(route, getBackedOffPoolSize(curr));
-            lastRouteBackoffs.put(route, now);
-        }
+            return lastUpdate;
+        });
     }
 
+
+    /**
+     * Returns the backed-off pool size based on the current pool size.
+     * The new pool size is calculated as the floor of (backoffFactor * curr).
+     *
+     * @param curr the current pool size
+     * @return the backed-off pool size, with a minimum value of 1
+     */
     private int getBackedOffPoolSize(final int curr) {
         if (curr <= 1) {
             return 1;
         }
-        return (int)(Math.floor(backoffFactor * curr));
+        return (int) (Math.floor(backoffFactor.get() * curr));
     }
 
+
     @Override
     public void probe(final HttpRoute route) {
-        synchronized(connPerRoute) {
-            final int curr = connPerRoute.getMaxPerRoute(route);
-            final int max = (curr >= cap) ? cap : curr + 1;
-            final Long lastProbe = getLastUpdate(lastRouteProbes, route);
-            final Long lastBackoff = getLastUpdate(lastRouteBackoffs, route);
-            final long now = clock.getCurrentTime();
-            if (now - lastProbe < coolDown.toMilliseconds()
-                || now - lastBackoff < coolDown.toMilliseconds()) {
-                return;
+        final int curr = connPerRoute.getMaxPerRoute(route);
+        final int max = (curr >= cap.get()) ? cap.get() : curr + 1;
+        final Instant now = Instant.now();
+
+        lastRouteProbes.compute(route, (r, lastProbe) -> {
+            if (lastProbe == null || now.isAfter(lastProbe.plus(Duration.ofMillis(coolDown.get().toMilliseconds())))) {
+                final Instant lastBackoff = lastRouteBackoffs.get(r);
+                if (lastBackoff == null || now.isAfter(lastBackoff.plus(Duration.ofMillis(coolDown.get().toMilliseconds())))) {

Review Comment:
   @arturobernalg It is a nitpick but one can avoid conversion to the `Duration` here. I think `lastProbe.plus(coolDown.get().toMilliseconds(), ChronoUnit.MILLIS)` should work the same way here with one less intermediate object on the heap.



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