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

[Daniel Fuchs]

Hi Andrej,

Yes that looks like my first implementation. But then
I reflected that avoiding to map Optional to Duration
and then back to a new Optional containing the same
duration could be avoided by simply storing the original
optional obtained from the HttpClient.

The current code only creates a new instance of Optional
when it needs to.

best regards,

-- daniel

On 18/01/2019 12:42, Andrej Golovnin wrote:
> 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