RFR: 8339538: Wrong timeout computations in DnsClient [v2]

[Daniel Fuchs]

On Mon, 9 Sep 2024 14:42:41 GMT, Aleksei Efimov <aefimov at openjdk.org> wrote:

>> src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java line 477:
>> 
>>> 475:                     long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(end - start);
>>> 476:                     // Setting the Math.clamp min to 1 ensures that the timeout is decreased
>>> 477:                     timeoutLeft = timeoutLeft - Math.clamp(elapsedMillis, 1, Integer.MAX_VALUE);
>> 
>> I think I'd prefer to calculate the remaining timeout based on the total elapsed time in this loop, as opposed to the time spent in blockingReceive.
>
> Sounds like a right thing to do:  measuring time in the loop should give us better estimation on time DNS client spends waiting on the response after submiting a query (that's how environment property value is defined in [javadoc here](https://docs.oracle.com/en/java/javase/22/docs/api/jdk.naming.dns/module-summary.html)).
> I've tried to move `start` and `end` like:
> 
>                  do {
> +                    long start = System.nanoTime();
> <....>
> -                    long start = System.nanoTime();
>                      gotData = blockingReceive(udpChannel, ipkt, timeoutLeft);
> -                    long end = System.nanoTime();
> <...>
> +                    long end = System.nanoTime();
>                      long elapsedMillis = TimeUnit.NANOSECONDS.toMillis(end - start);
> 
> As a result the tests showed that the timeout happened with a bit better precision (10th of milliseconds).
> I will run more tests and incorporate the suggestions. Thank you.

I have been wondering why the timeout was measured in this way. My explanation was that the original author of the API (duke? ;-) ) wanted to measure "actual timeout" (time actually spent waiting for a packet) as opposed to perceived timeout (time the caller spent waiting). Rationale might have been to exclude potential time spent waiting for a lock before entering receive() - for instance. That said - I'm not opposed to extend the scope of the timeout to make it closer to the perceived timeout, which is what the test expect. 
If we do this maybe we could move `start` out of the `do` block  - which would make the computation of `timeoutLeft` simpler (we wouldn't need the `-=`).

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20892#discussion_r1750438221