On Fri, 15 Oct 2021 14:25:02 GMT, Daniel Fuchs <dfu...@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
