RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]

[Aleksei Efimov]

On Fri, 15 Oct 2021 14:25:02 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Aleksei Efimov has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Add @since tags to new API classes
>>  - Add checks and test for empty stream resolver results
>
> src/java.base/share/classes/java/net/InetAddress.java line 231:
> 
>> 229:  *      The first provider found will be used to instantiate the {@link InetAddressResolver InetAddressResolver}
>> 230:  *      by invoking the {@link InetAddressResolverProvider#get(InetAddressResolverProvider.Configuration)}
>> 231:  *      method. The instantiated {@code InetAddressResolver} will be installed as the system-wide
> 
> Maybe "... method. The **returned** {@code InetAddressResolver} will be installed ..."

Changed as suggested in 648e65b8e6ae9687f7164ea3e81c51ab2b5e2dfe.

> src/java.base/share/classes/java/net/InetAddress.java line 486:
> 
>> 484:                 return cns;
>> 485:             } finally {
>> 486:                 bootstrapResolver = null;
> 
> This is incorrect. This will set bootstrapResolver to null in the case where you return it at line 468 - which means that a second call will then find it null. I believe you want to set it to null in the finally clause only if you reached line 470. You could achieve this by declaring a local variable - e.g `InetAddressResolver tempResolver = null;` before entering the try-finally then set that variable at line 470 `return tempResolver = bootstrapResolver;` and in finally do `if (tempResolver == null) bootsrapResolver = null;`
> 
> To test this you could try to call e.g. `InetAddress.getByName` twice in succession in your test: I believe it will fail with the code as it stands, but will succeed with my proposed changes...

Good catch, thank you Daniel. Added a test for that and fixed it in cd19ecd63838d3c2b713ed66d41107fe01a2d3e6. Now the `bootstrapResolver` field is set back to `null` in finally block only if a call to `InetAddress.resolver()` entered a resolver initialization part.

> src/java.base/share/classes/java/net/InetAddress.java line 860:
> 
>> 858:                 return host;
>> 859:             }
>> 860:         } catch (RuntimeException | UnknownHostException e) {
> 
> This may deserve a comment: resolver.lookUpHostName and InetAddress.getAllbyName0 delegate to the system-wide resolver, which could be custom, and we treat any unexpected RuntimeException thrown by the provider at that point as we would treat an UnknownHostException or an unmatched host name.

Agree - comment added as part of 648e65b8e6ae9687f7164ea3e81c51ab2b5e2dfe.

> src/java.base/share/classes/java/net/InetAddress.java line 1063:
> 
>> 1061:         public Stream<InetAddress> lookupAddresses(String host, LookupPolicy policy)
>> 1062:                 throws UnknownHostException {
>> 1063:             Objects.requireNonNull(host);
> 
> Should we also double check that `policy` is not null? Or maybe assert it?

Yes, we should since custom resolvers might call the built-in one with `null` - added non-null check in 648e65b .

> src/java.base/share/classes/java/net/InetAddress.java line 1203:
> 
>> 1201:                         + " as hosts file " + hostsFile + " not found ");
>> 1202:             }
>> 1203:             // Check number of found addresses:
> 
> I wonder if the logic here could be simplified by first checking whether only IPv4 addresses are requested, and then if only IPv6 addresses are requested. 
> 
> That is - evacuate the simple cases first and then only deal with the more complex case where you have to combine the two lists...

Tried to simplify it in 648e65b8e6ae9687f7164ea3e81c51ab2b5e2dfe

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

PR: https://git.openjdk.java.net/jdk/pull/5822