Enhance usage of FQDN in PoolKey

[yuzawa-san]

The SocketAddres.toString method to generate the FQDN is relatively heavy and it appears to be unnecessary given the socket address is being hashed and equaled already. This should work for unix domain sockets as well.

This code:

System.out.println(AddressUtils.createUnresolved("127.0.0.1", 80));
System.out.println(AddressUtils.createUnresolved("127.0.0.1", 80).equals(AddressUtils.createUnresolved("127.0.0.1", 80)));
System.out.println(AddressUtils.createUnresolved("example.com", 80));
System.out.println(AddressUtils.createUnresolved("example.com", 80).equals(AddressUtils.createUnresolved("example.com", 80)));
System.out.println(AddressUtils.createResolved("example.com", 80));
System.out.println(AddressUtils.createResolved("example.com", 80).equals(AddressUtils.createResolved("example.com", 80)));
System.out.println(AddressUtils.createResolved("example.com", 80).equals(AddressUtils.createUnresolved("example.com", 80)));
System.out.println(UnixDomainSocketAddress.of("/tmp/mysock"));

prints out:

/127.0.0.1:80
true
example.com/<unresolved>:80
true
example.com/93.184.216.34:80
true
false
/tmp/mysock

before:

and after this is basically gone

I also fixed up the hashCode like I did in #2732

[violetagg]

@yuzawa-san Can you please elaborate more on how we will handle different subdomains?

[yuzawa-san]

@violetagg I set a breakpoint in the existing code when my client hits https://en.wikipedia.org/ and https://wikipedia.org/ and the socket (toString) is en.wikipedia.org/<unresolved>:443 and wikipedia.org/<unresolved>:443 respectively

System.out.println(AddressUtils.createUnresolved("wikipedia.org", 443).equals(AddressUtils.createUnresolved("en.wikipedia.org", 443)));

prints false which is expected, indicating those would go into two separate pools which is correct.

is the concern that some resolved InetSocketAddresses could be bucketed in the pool together? like weird mixtures of resolved and unresolved InetSocketAddresses or ip-address only vs hostname only InetSocketAddresses? or would it be inconsistent across different java versions? looks like it would work: https://github.com/openjdk/jdk/blob/jdk-11%2B0/src/java.base/share/classes/java/net/InetSocketAddress.java#L112

I can post some more mixtures shortly if you need more concrete examples

[violetagg]

Can you try this?

HttpClient client = HttpClient.create().wiretap(true);

client.remoteAddress(() -> new InetSocketAddress("wikipedia.org", 443)).get().uri("/").responseContent().aggregate().asString().block();

client.remoteAddress(() -> new InetSocketAddress("en.wikipedia.org", 443)).get().uri("/").responseContent().aggregate().asString().block();

https://httpwg.org/specs/rfc7540.html#reuse

[yuzawa-san]

@violetagg my original traces all involved unresolved InetSocketAddresses, but the wikipedia examples are a prime example of where the resolved addresses are the same and thus equals and hashcode bucket them into the same pool key. i found a solution which only uses FQDN for tiebreaking when the InetSocketAddress has a resolved address.

this has a fast path still for the unresolved addresses coming out of the UriEndpointFactory. since those are unresolved, we can skip the fqdn and lean directly on the equals and hashcode of InetSocketAddress.

i also added a test case. i found that the holder.toString() actually did not mimic the toLowerCase behavior present in those links, so maybe that is an extra bugfix i stumbled upon. see key3 and key4, those were true in the prior version of the code.

[violetagg]

@yuzawa-san Thanks

only two small comments

[violetagg]

@reactor/netty-team PTAL

[pderop]

@yuzawa-san , thanks for the PR.

[violetagg]

Thanks