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

Mark Sheppard msheppar at openjdk.org
Tue Sep 10 09:52:06 UTC 2024


On Mon, 9 Sep 2024 22:29:23 GMT, Aleksei Efimov <aefimov at openjdk.org> wrote:

>> This PR proposes the following changes to address wrong timeout computations in the `com.sun.jndi.dns.DnsClient`:
>> - The `DnsClient` has been updated to use a monotonic high-resolution (nano) clock. The existing `Timeout` test has also been updated to use the nano clock to measure observed timeout value.
>> 
>> - The left timeout computation has been fixed to decrease the timeout value during each retry attempt. A new test, `TimeoutWithEmptyDatagrams`, has been added to test it.
>> 
>> - The `DnsClient.blockingReceive` has been updated:
>>     - to detect if any data is received
>>     - to avoid contention with `Selector.close()` that could be called by a cleaner thread
>> 
>> - The expected timeout calculation in the `Timeout` test has been updated to take into account the minimum retry timeout (50ms). Additionally, the max allowed difference between the observed timeout and the expected one has been increased from 50% to 67%. Taking into account 50 ms retry timeout decrease the maximum allowed difference is effectively set to 61%. This change is expected to improve the stability of the `Timeout` test which has been seen to fail [intermittentlly](https://bugs.openjdk.org/browse/JDK-8220213). If no objections, I'm planning to close [JDK-8220213](https://bugs.openjdk.org/browse/JDK-8220213) as duplicate of this one.
>> 
>> JNDI/DNS jtreg tests has been executed multiple times (500+) to check if the new and the modified tests are stable. No failures been observed (so far?).
>
> Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Measure time the caller spent waiting. Simplify timeoutLeft computation

test/jdk/com/sun/jndi/dns/ConfigTests/Timeout.java line 112:

> 110:             // Check that elapsed time is as long as expected, and
> 111:             // not more than 67% greater. Given the min DNS timeout
> 112:             // correction above the threshold value is equal to 61%.

this is a bit arcane, why not have a simple measure that elapsed time shouldn't be more than twice the expected timeout ... this is not that different to the  multipliedBy(2) and multipliedBy(3) --  elaspedTime.compareTo(expectedTime.multipliedBy(2) <= 0

Additionally based on the internal minimum timeout allowance of 50 secs and this upper bound calculation, it would suggest that an @implNote might be useful, or required, to alert developers  to potential timeout variability, and not to rely on timeout to be absolutely (real time) precise

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

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


More information about the core-libs-dev mailing list