8244202: Implementation of JEP 418: Internet-Address Resolution SPI

[AlekseiEfimov]

This change implements a new service provider interface for host name and address resolution, so that java.net.InetAddress API can make use of resolvers other than the platform's built-in resolver.

The following API classes are added to java.net.spi package to facilitate this:

• InetAddressResolverProvider - abstract class defining a service, and is, essentially, a factory for InetAddressResolver resolvers.
• InetAddressResolverProvider.Configuration - an interface describing the platform's built-in configuration for resolution operations that could be used to bootstrap a resolver construction, or to implement partial delegation of lookup operations.
• InetAddressResolver - an interface that defines methods for the fundamental forward and reverse lookup operations.
• InetAddressResolver.LookupPolicy - a class whose instances describe the characteristics of one forward lookup operation.

More details in JEP-418.

Testing: new and existing tier1:tier3 tests

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8244202: Implementation of JEP 418: Internet-Address Resolution SPI

Reviewers

• Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to f660cc6
• Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 1378686
• Michael McMahon (@Michael-Mc-Mahon - Reviewer) ⚠️ Review applies to f660cc6
• Chris Hegarty (@ChrisHegarty - Reviewer) ⚠️ Review applies to 0542df5

Contributors

• Chris Hegarty <chegar@openjdk.org>
• Daniel Fuchs <dfuchs@openjdk.org>
• Alan Bateman <alanb@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822
$ git checkout pull/5822

Update a local copy of the PR:
$ git checkout pull/5822
$ git pull https://git.openjdk.java.net/jdk pull/5822/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5822

View PR using the GUI difftool:
$ git pr show -t 5822

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5822.diff

[bridgekeeper]

👋 Welcome back aefimov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@AlekseiEfimov The following labels will be automatically applied to this pull request:

• core-libs
• net
• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 12: Full (a2498d2b)
• 11: Full (0542df51)
• 10: Full - Incremental (b31d61d1)
• 09: Full - Incremental (f660cc6e)
• 08: Full - Incremental (2ca78ba9)
• 07: Full - Incremental (1378686b)
• 06: Full - Incremental (fa655be2)
• 05: Full - Incremental (2a554c91)
• 04: Full - Incremental (30226481)
• 03: Full - Incremental (ac0d2184)
• 02: Full - Incremental (d302a49a)
• 01: Full - Incremental (41717fc7)
• 00: Full (77551538)

[msheppar]

What’s in a name?
I find the method names of the InetAddressResolver interface a bit ambiguous. Typically in this DNS problem space
one speaks of lookup to resolve a hostname to its associated addresses and reverse lookup
to resolve an IP address to a hostname (or names) associated with it.

For the method names lookupHostname, and lookupAddresses,
and the semantics conveyed in those names,
I would expect lookupHostname to resolve a hostname, that is, take a hostname as parameter and
the return value the addresses, while lookupAddresses I would expect to resolve an address to its hostname,
that is, take an address and return a hostname.
However, the current pair of methods names convey the opposite, lookupHostname resolves an address and
lookupAddresses resolves a hostname.

I would proffer a method name change to use lookup and reverseLookup
OR to use resolvesHostname and resolveAddress, to resolve a hostname and address
respectively.

Thus, lookupHostname should be renamed to reverseLookup and
lookupAddresses to lookup OR
lookupHostname renamed to resolveAddress and lookupAddresses renamed to resolveHostname.

[msheppar]

Refactor remove Configuration and simplify interface:

In the InetAddressResolver a Configuration abstraction is defined, and this is supplied to
a get method of the InetAddressResolverProvider.

The objective is to “inject” some platform configuration into an InetAddressResolver. This
consistents of two pieces of platform configuration detail, a builtin resolver reference and the hostname.
Why not provide them as parameters to the provider get method and dispense with the Configuration
abstraction? Thus simplifying the interface. The hostname is being supplied by the getLocalHostName
of an InetAddressImpl. This is essentially a wrapper to the library call gethostname. This
could be provided as a static method in InetAddressImpl, called once during loadResolver to
provide the hostname parameter String. The hostname is a static piece of system configuration, which
requires a system restart if it were to be changed - so need to provide a lambda.

So Suggestion is refector remove Configuration to simplify the interface and provide the
BULITIN_RESOLVER and hostname as parameters to the InetAddressResolverProvider::get method

[AlekseiEfimov]

So Suggestion is refector remove Configuration to simplify the interface and provide the BULITIN_RESOLVER and hostname as parameters to the InetAddressResolverProvider::get method

During work on this JEP we've examined the approach suggested above, and there are few issues with it:

• InetAddressResolverProvider.Configuration interface API might evolve, e.g. there might be a need to pass additional configuration items.
• local hostname is a dynamic information, therefore it cannot be passed as string parameter to InetAddressResolverProvider::get.

It's been also discussed in the CSR comments here

[AlekseiEfimov]

What’s in a name? I find the method names of the InetAddressResolver interface a bit ambiguous. Typically in this DNS problem space one speaks of lookup to resolve a hostname to its associated addresses and reverse lookup to resolve an IP address to a hostname (or names) associated with it.

I'm not sure that I see an ambiguity here:
Interface has two methods, both are for lookup operations. That is why lookup word is used in both names. Then we put a description of a returned result after operation name: Addresses and HostName are the results. In cases when we want to highlight a parameter type in a method name we can use ‘by’/‘with’ prepositions: for instance, InetAddress.getByName InetAddress.getByAddress ScheduledExecutorService.scheduleWithFixedDelay.

We can try and apply this approach to the description of DNS operations above:

lookup to resolve a hostname to its associated addresses

operation: lookup result: addresses -> methodName: lookupAddresses

reverse lookup to resolve an IP address to a hostname

operation: lookup (we've dropped reverse word here) result: hostname -> methodName: lookupHostName

[AlanBateman]

• InetAddressResolverProvider.Configuration interface API might evolve, e.g. there might be a need to pass additional configuration items.
• local hostname is a dynamic information, therefore it cannot be passed as string parameter to InetAddressResolverProvider::get.

Right, I think we've ended up with a good model here that supports the common case, stacking, and can be extended in the future to add new configuration if required. Also if good use-cases come up when Configuration can be changed to be non-sealed.

[msheppar]

What’s in a name? I find the method names of the InetAddressResolver interface a bit ambiguous. Typically in this DNS problem space one speaks of lookup to resolve a hostname to its associated addresses and reverse lookup to resolve an IP address to a hostname (or names) associated with it.

I'm not sure that I see an ambiguity here: Interface has two methods, both are for lookup operations. That is why lookup word is used in both names. Then we put a description of a returned result after operation name: Addresses and HostName are the results. In cases when we want to highlight a parameter type in a method name we can use ‘by’/‘with’ prepositions: for instance, InetAddress.getByName InetAddress.getByAddress ScheduledExecutorService.scheduleWithFixedDelay.

We can try and apply this approach to the description of DNS operations above:

lookup to resolve a hostname to its associated addresses

operation: lookup result: addresses -> methodName: lookupAddresses

reverse lookup to resolve an IP address to a hostname

operation: lookup (we've dropped reverse word here) result: hostname -> methodName: lookupHostName

Yes, ambiguity wasn’t quite the word I was looking for. Maybe contradictory or oxymoron might do !!
Let me try and clarify the rational behind the suggestion.

To re-iterate, the parlance of the DNS domain in context of address resolution is lookup for hostname to IP address resolution and reverse lookup for address to hostname mapping. For me, the natural verb to method translation is lookup and **reverseLookup OR resolveHostname and resolveAddress. The latter names are pretty definitive and unambiguous. It clear and obvious without the need to pursue the full signature of the method, what the semantics of the methods should be.

At the java InetAddress API level there is getByName(String host) and getByAddress(byte[] addr).
At the native OS level there is gethostbyname, gethostbyaddr and the evolved api getnameinfo and getaddrinfo.

getByName requires a hostname lookup and getByAdress requires (eventually - I know the docs says there’s no reverse lookup) an address reverse lookup. Thus, a logical mapping is getByName —> lookupHostname, and getByAddr —> lookupAddress, but the opposite will occur getByName --> lookupAddresses and getByAddress --> lookupHostname. Therfore I proffer maps neatly to rename methods to resolveHostname and resolveAddress such that the mapping are getByName --> resolveHostname and getByAddress --> resolveAddress.

I will concede that it can be reasonable argued that the native library functions getnameinfo and getaddrinfo may better abstract to lookupHostname and lookupAddresses respectively. But, I have always found these two library calls a bit contorted and the legacy gethostbyname and gethostbyaddress were more logical.

In you proposition above:

operation: lookup result: addresses -> methodName: lookupAddresses

operation: lookup (we've dropped reverse word here) result: hostname -> methodName: lookupHostName

You are generating the name from the result of the action. I’m arguing that it should be based on the operation on the subject i.e. the parameter being passed.
If this was hashmap lookup, then it is an operation of mapping hostname to addresses mapHostnameToAddresses, i.e. looking up a hostname, and mapping an address to a hostname mapAddressToHostname i.e. looking up an address.

Of course, it is possible to just overload a lookup method, one taking Sting and t’other taking byte adder in the method’s parameters, respectively. Then, you don’t have to concede too much in this argument.
But, for me as an ordinary developer, resolveAddress and resolveHostname are crystal clear and unambiguous.

[msheppar]

So Suggestion is refector remove Configuration to simplify the interface and provide the BULITIN_RESOLVER and hostname as parameters to the InetAddressResolverProvider::get method

During work on this JEP we've examined the approach suggested above, and there are few issues with it:

* `InetAddressResolverProvider.Configuration` interface API might evolve, e.g. there might be a need to pass additional configuration items.

* local hostname is a dynamic information, therefore it cannot be passed as string parameter to `InetAddressResolverProvider::get`.

It's been also discussed in the CSR comments here

OK grand ... I didn't see the CSR discussion :-(

WRT hostname being dynamic and requires a lambda ... is it really dynamic?
I know it can be changed, but IIRC most (unix and windows) if not all systems require a restart for such a change change to take effect, and usually requires a dns cache flush also, in which case an application will be restarting also.

In the case of working over VPN, VPN software usually changes the hostname dynamically, but that name change is only valid for the duration of the VPN session, and when you exit the VPN, it reverts back to the original hostname. In such cases an application will need to be restarted. There's no seemless working from VPN to non VPN.

I think that a hostname is constant while a host is up, but it can be changed, and when changed a host restart is required. I don't think it is quite as dynamic as has been suggested, but I open to correction.

It is something that can be investigated.

[AlekseiEfimov]

I think that a hostname is constant while a host is up, but it can be changed, and when changed a host restart is required. I don't think it is quite as dynamic as has been suggested, but I open to correction.

It is possible to change a host name without host restart. What has been tested:

• Checked O/S type - Linux
• hostnamectl cmd was used to change a host name
• In output below there is only one jshell instance running.

jshell> InetAddress.getLocalHost()
$1 ==> test-host-name/192.168.0.155

$ hostnamectl set-hostname 'test-host-name-changed'

# Need to sleep for 5 seconds since local host
# is cached for 5 seconds since last resolution
$ sleep 5

jshell> InetAddress.getLocalHost()
$2 ==> test-host-name-changed/192.168.0.155

I believe it proves that we need to treat a host name as dynamically changing information.

[dfuch]

Changes look good to me.

[openjdk]

@AlekseiEfimov This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8244202: Implementation of JEP 418: Internet-Address Resolution SPI

Co-authored-by: Chris Hegarty <chegar@openjdk.org>
Co-authored-by: Daniel Fuchs <dfuchs@openjdk.org>
Co-authored-by: Alan Bateman <alanb@openjdk.org>
Reviewed-by: dfuchs, alanb, michaelm, chegar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 15 new commits pushed to the master branch:

• aea0967: 8275854: C2: assert(stride_con != 0) failed: missed some peephole opt
• 9862cd0: 8275786: New javadoc option to add script files to generated documentation
• 7a140af: 8276546: [IR Framework] Whitelist and ignore CompileThreshold
• 91bb0d6: 8276796: gc/TestSystemGC.java large pages subtest fails with ZGC
• 08e0fd6: 8276835: G1: make G1EvacFailureObjectsSet::record inline
• ad3be04: 8276536: Update TimeZoneNames files to follow the changes made by JDK-8275766
• e27a67a: 8276930: Update ProblemList
• 73e6d7d: 8276801: gc/stress/CriticalNativeStress.java fails intermittently with Shenandoah
• bce35ac: 8276775: ZonedDateTime/OffsetDateTime.toString return invalid ISO-8601 for years <= 1893
• 0c409ca: 8276186: Require getAvailableLocales() methods to include Locale.ROOT
• ... and 5 more: https://git.openjdk.java.net/jdk/compare/f561d3c1942ce901fa77c907839032f76feb6998...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[AlekseiEfimov]

/csr needed

[AlanBateman]

Thanks for the updates to the API docs, you've addressed all my comments.

[openjdk]

@AlekseiEfimov this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8244202_JEP418_impl
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[ChrisHegarty]

LGTM.

[AlekseiEfimov]

/contributor add chegar
/contributor add dfuchs
/contributor add alanb

[openjdk]

@AlekseiEfimov
Contributor Chris Hegarty <chegar@openjdk.org> successfully added.

[openjdk]

@AlekseiEfimov
Contributor Daniel Fuchs <dfuchs@openjdk.org> successfully added.

[openjdk]

@AlekseiEfimov
Contributor Alan Bateman <alanb@openjdk.org> successfully added.

[AlekseiEfimov]

/integrate

[openjdk]

Going to push as commit 2ca4ff8.
Since your change was applied there have been 16 commits pushed to the master branch:

• c29cab8: 8276112: Inconsistent scalar replacement debug info at safepoints
• aea0967: 8275854: C2: assert(stride_con != 0) failed: missed some peephole opt
• 9862cd0: 8275786: New javadoc option to add script files to generated documentation
• 7a140af: 8276546: [IR Framework] Whitelist and ignore CompileThreshold
• 91bb0d6: 8276796: gc/TestSystemGC.java large pages subtest fails with ZGC
• 08e0fd6: 8276835: G1: make G1EvacFailureObjectsSet::record inline
• ad3be04: 8276536: Update TimeZoneNames files to follow the changes made by JDK-8275766
• e27a67a: 8276930: Update ProblemList
• 73e6d7d: 8276801: gc/stress/CriticalNativeStress.java fails intermittently with Shenandoah
• bce35ac: 8276775: ZonedDateTime/OffsetDateTime.toString return invalid ISO-8601 for years <= 1893
• ... and 6 more: https://git.openjdk.java.net/jdk/compare/f561d3c1942ce901fa77c907839032f76feb6998...master

Your commit was automatically rebased without conflicts.

[openjdk]

@AlekseiEfimov Pushed as commit 2ca4ff8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.