RFR: 8304885: Reuse stale data to improve DNS resolver resiliency [v8]

[Daniel Fuchs]

On Wed, 10 May 2023 21:58:52 GMT, Sergey Bylokhov <serb at openjdk.org> wrote:

>> I would like to get preliminary feedback about the provided patch.
>> 
>> Discussion on net-dev@ https://mail.openjdk.org/pipermail/net-dev/2023-March/020682.html
>> 
>> One of the main issue I try to solve is how the cache handle the intermittent DNS server outages due to overloading or network connection.
>> 
>> At the moment this cache can be configured by the application using the following two properties:
>>    (1) "networkaddress.cache.ttl"(30 sec) - cache policy for successful lookups
>>    (2) "networkaddress.cache.negative.ttl"(10 sec) - cache policy for negative lookups
>> 
>> The default timeout for positive responses is good enough to "have recent dns-records" and to "minimize the number of requests to the DNS server".
>> 
>> But the cache for the negative responses is problematic. This is a problem I would like to solve. Caching the negative response means that for **10** seconds the application will not be able to connect to the server.
>> 
>> Possible solutions:
>>   1. Decreasing timeout "for the negative responses": unfortunately more requests to the server at the moment of "DNS-outage" cause even more issues, since this is not the right moment to load the network/server more.
>>   2. Increasing timeout "for the positive responses": this will decrease the chance to get an error, but the cache will start to use stale data longer.
>>   3. This proposal: it would be good to ignore the negative response and continue to use the result of the last "successful lookup" until some additional timeout.
>> 
>> The idea is to split the notion of the TTL and the timeout used for the cache. When TTL for the record will expire we should request the new data from the server. If this request goes fine we will update the record, if it fails we will continue to use the cached date until the next sync.
>> 
>> For example, if the new property "networkaddress.cache.extended.ttl" is set to 10 minutes, then we will cache a positive response for 10 minutes but will try to sync it every 30 seconds. If the new property is not set then as before we will cache positive for 30 seconds and then cache the negative response for 10 seconds.
>> 
>> 
>> RFC 8767 Serving Stale Data to Improve DNS Resiliency:
>> https://www.rfc-editor.org/rfc/rfc8767
>> 
>> Comments about current and other possible implementations:
>>  * The code intentionally moved to the separate ValidAddresses class, just to make clear that the default configuration, when the new property is not set is not changed much.
>>  * The refr...
>
> Sergey Bylokhov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Apply suggestions from code review
>   
>   Co-authored-by: Michael McMahon <70538289+Michael-Mc-Mahon at users.noreply.github.com>

src/java.base/share/classes/java/net/InetAddress.java line 215:

> 213:  * it were unsuccessful. This property is useful if it is preferable to use a
> 214:  * stale name rather than result of unsuccessful name lookup. The default
> 215:  * setting is to cache for an implementation specific period of time.

AFAICS the default setting is to attempt to refresh stale data immediately and not cache them.

src/java.base/share/classes/java/net/InetAddress.java line 966:

> 964:      * addresses or invalid (ie a failed lookup) containing no addresses.
> 965:      */
> 966:     private static class CachedLookup implements Addresses, Comparable<CachedLookup> {

I do not really like the fact that CachedAddress -> CachedLookup has transformed an immutable class into a mutable one, which in addition forces a volatile read to get the addresses.

Have you explored moving the new functionality implemented by `ValidCachedLookup` inside `NameServiceAddresses`?

Couldn't NameServiceAddresses be initialized with a (final) InetAddress[] array (from the previous stale lookup) that, if not null, would be returned when `tryLock` returns false?

In other words if NameServiceAddresses::staleAddresses is not null, and we are still in the stale window, use tryLock, and if false, return staleAddresses; otherwise, use lock.

Would that work, and keep everything final instead of volatile?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13285#discussion_r1196558484
PR Review Comment: https://git.openjdk.org/jdk/pull/13285#discussion_r1196639624