Re: [13] RFR: 8216561: HttpClient: The logic of retry on connect exception is inverted

[Andrej Golovnin]

Hi Daniel,

> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8216561/webrev.01/
>
>
>
 126     private static class ConnectTimeoutTracker {
 127         final Optional<Duration> max;
 128         final AtomicLong startTime = new AtomicLong();
 129         ConnectTimeoutTracker(Optional<Duration> connectTimeout) {
 130             this.max = connectTimeout;
 131         }
 132
 133         Optional<Duration> getRemaining() {
 134             if (max.isEmpty()) return max;
 135             long now = System.nanoTime();
 136             long previous = startTime.compareAndExchange(0, now);
 137             if (previous == 0 || max.get().isZero()) return max;
 138             Duration remaining = max.get().minus(Duration.ofNanos(now
- previous));
 139             assert remaining.compareTo(max.get()) <= 0;
 140             return Optional.of(remaining.isNegative() ? Duration.ZERO
: remaining);
 141         }
 142
 143         void reset() { startTime.set(0); }
 144     }

An Optional in a class field or in a data structure, is considered a misuse
of the API (see the explanation by Stuart Marks:
https://stackoverflow.com/questions/23454952/uses-for-optional/23464794#23464794).
I would write it this way:

      private static class ConnectTimeoutTracker {
          final Duration max;
          final AtomicLong startTime = new AtomicLong();
          ConnectTimeoutTracker(Duration connectTimeout) {
              this.max = connectTimeout;
          }

          Optional<Duration> getRemaining() {
              return Optional.ofNullable(max)
                  .map(m -> {
                      long now = System.nanoTime();
                      long previous = startTime.compareAndExchange(0, now);
                      if (previous == 0 || m.isZero()) {
                          return m;
                      }
                      Duration remaining = m.minus(Duration.ofNanos(now -
previous));
                      assert remaining.compareTo(m) <= 0;
                      return remaining.isNegative() ? Duration.ZERO :
remaining;
                  });
          }

          void reset() { startTime.set(0); }
      }

Best regards,
Andrej Golovnin