On Wed, 17 May 2023 13:51:23 GMT, Daniel Fuchs <[email protected]> wrote:
>> 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 >> <[email protected]> > > 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. It is only true if the security property is not set for some specific binaries, and it can have some different value than zero by default. > 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? This is what I tried to implement initially: NameServiceAddresses which stores the fallback value of staleAddresses. Unfortunately the NameServiceAddresses class is [used](https://github.com/openjdk/jdk/blob/aad899b7f964c4c4f05ad35208c58118692c9aa0/src/java.base/share/classes/java/net/InetAddress.java#L1808) when we already deleted the addresses from the caches. So the first thread may save the old stale data from the cache, delete and create a new "proper" NameServiceAddresses, but another thread may do the same and get the "empty" old stale data -> if the second request will be faster we will lost the stale data. It could be fixed by some additional synchronization, but that would be more expensive than one volatile read. Note that we can eliminate that one volatile read by duplicating the logic of CachedLookup inside ValidCachedLookup by using one interface for both, not sure that it is worth, since we already have a couple of loookups to the ConcurrentHashMap and ConcurrentSkipListSet on each call. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13285#discussion_r1196928240 PR Review Comment: https://git.openjdk.org/jdk/pull/13285#discussion_r1196926931
