Implement HappyEyeballsV2AsyncClientConnectionOperator #428

arturobernalg · 2023-03-24T21:57:36Z

This pull request adds a new implementation of the Happy Eyeballs algorithm for asynchronous client connections in Apache's HttpClient component. The new implementation, HappyEyeballsV2AsyncClientConnectionOperator, builds on the previous version by improving support for IPv6 and providing more fine-grained control over connection timing.

The changes include:

Addition of HappyEyeballsV2AsyncClientConnectionOperator class and related test classes
Modifications to existing code to use the new operator where appropriate
Documentation updates
Allow activate HappyEyeballsV2Async via Fast Fallback var in PoolingAsyncClientConnectionManagerBuilder.java

ok2c · 2023-03-27T10:57:13Z

@arturobernalg Allow me a few days to review.

ok2c · 2023-04-03T06:36:06Z

@arturobernalg Looks good to me. @rschmitt Would you also like to review?

rschmitt · 2023-04-04T03:58:31Z

...n/java/org/apache/hc/client5/http/impl/nio/HappyEyeballsV2AsyncClientConnectionOperator.java

+                // Introduce a delay before resolving AAAA records after receiving A records
+                resolution_delay.sleep();
+                dnsFuture.complete(inetAddresses);
+            } catch (final UnknownHostException | InterruptedException e) {


Shouldn't you catch Exception here, instead of just the checked exceptions?

rschmitt · 2023-04-04T03:59:10Z

...n/java/org/apache/hc/client5/http/impl/nio/HappyEyeballsV2AsyncClientConnectionOperator.java

+            try {
+                final InetAddress[] inetAddresses = dnsResolver.resolve(host);
+                // Introduce a delay before resolving AAAA records after receiving A records
+                resolution_delay.sleep();


I'm slightly concerned about async code, running in the shared ForkJoinPool, performing a blocking sleep of a configurable duration.

Instead of using Thread.sleep(), i could use non-blocking methods(ScheduledExecutorService) to delay the execution of the next connection attempt. wdyt?

Yes, in async code delays should always be implemented by yielding the current thread and scheduling a continuation, rather than calling Thread.sleep. Ideally, the DNS lookup would also be made as an async call, but to my knowledge there's really nothing we can do about that, and in practical terms DNS is very fast. I'm not even sure Netty has an async implementation of DNS lookups.

Netty does, but it's an entirely bespoke implementation that doesn't touch the JDK's built-int resolver. The blocking here will hopefully become less of a problem for this client once Loom is available.

rschmitt · 2023-04-04T04:02:48Z

...n/java/org/apache/hc/client5/http/impl/nio/HappyEyeballsV2AsyncClientConnectionOperator.java

+            if (i < addresses.size() - 1) {
+                try {
+                    final Duration delay = calculateDelay(i);
+                    delay.wait();


What thread is this code running in? This will block for up to 2,000 milliseconds.

The RFC recommends introducing a delay before starting the next connection attempt, but it doesn't mandate a specific implementation. The RFC suggests a default delay of 250 milliseconds, but also notes that a more nuanced implementation can use historical RTT data to influence the delay. Therefore, it is up to the implementation to decide whether to introduce a delay and how to calculate that delay.
Maybe would be better to use a non-blocking delay mechanism, such as Thread.sleep() or a ScheduledExecutorService. wdyt?

Wait a minute, this isn't even a sleep call, this is calling Object::wait!! This isn't correct at all and may put the thread to sleep for good if nothing calls notifyAll on the same Duration instance. Is this code even tested?

rschmitt · 2023-04-04T04:09:28Z

...org/apache/hc/client5/http/impl/nio/HappyEyeballsV2AsyncClientConnectionOperatorBuilder.java

+        final DnsResolver dnsResolverToUse = dnsResolver != null ? dnsResolver : SystemDefaultDnsResolver.INSTANCE;
+        final Timeout timeoutToUse = timeout != null ? timeout : Timeout.ofMilliseconds(250);
+        final Timeout resolutionDelayToUse = resolutionDelay != null ? resolutionDelay : Timeout.ofMilliseconds(50);
+        final Timeout minimumConnectionAttemptDelayToUse = minimumConnectionAttemptDelay != null ? minimumConnectionAttemptDelay : Timeout.ofMilliseconds(100);
+        final Timeout maximumConnectionAttemptDelayToUse = maximumConnectionAttemptDelay != null ? maximumConnectionAttemptDelay : Timeout.ofSeconds(2);
+        final Timeout connectionAttemptDelayToUse = connectionAttemptDelay != null ? connectionAttemptDelay : Timeout.ofMilliseconds(250);
+        final int firstAddressFamilyCountToUse = Args.positive(firstAddressFamilyCount, "First Address Family");
+        final HappyEyeballsV2AsyncClientConnectionOperator.AddressFamily addressFamilyToUse = addressFamily;


These defaults are all duplicated in the HappyEyeballsV2AsyncClientConnectionOperator constructor.

rschmitt · 2023-04-04T04:49:33Z

httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/InetAddressComparator.java

+        if (adr1 instanceof Inet4Address && adr2 instanceof Inet6Address) {
+            return -1;
+        }
+        // Compare based on address bytes


This appears to have nothing to do with the address sorting algorithm specified by RFC 6724, which is mandated by RFC 8305. Furthermore, sorting addresses in this way is a bad idea, since it will defeat round-robin DNS, an important load balancing technique.

rschmitt · 2023-04-04T04:56:21Z

...va/org/apache/hc/client5/http/impl/nio/HappyEyeballsV2AsyncClientConnectionOperatorTest.java

+
+    }
+
+    @DisplayName("Test that application prioritizes IPv6 over IPv4 when both are available")


Again, the sorting rules are more complicated than this. For example, native IPv4 addresses are preferred to IPv6 addresses that use an encapsulation/tunneling transport like 6to4 or Teredo. IPv6 addresses will also be considered unusable, and therefore deprioritized, if no IPv6 source address is available.

rhernandez35 · 2023-04-04T23:29:50Z

...n/java/org/apache/hc/client5/http/impl/nio/HappyEyeballsV2AsyncClientConnectionOperator.java

+                    scheduler.schedule(() -> {
+                        try {
+                            final InetAddress[] inet6Addresses = dnsResolver.resolve(host);
+                            dnsFuture.complete(inet6Addresses);


You can't complete dnsFuture twice. The complete call on line 408 will always put dnsFuture into a finished state.

And please don't use obtrudeValue to work around complete being one-and-done

arturobernalg · 2023-04-07T21:38:21Z

Hi @rschmitt
I have implemented Destination Address Selection and improved the handling of async connections in HappyEyeballsV2AsyncClientConnectionOperator.
I have also added a new class to validate the HappyEyeballsV2 rules: HappyEyeballsV2RulesTest. Thought you might find it useful.

Let me know if you have any feedback. Thanks!

rschmitt

I think a lot of the destination address sorting rules need to be reimplemented in terms of local address availability. It's also important to make sure we are using a stable sort. Finally, the compareTo method can't make any network calls.

These rules are complex enough that they really need dedicated unit testing. Generative testing may be a good idea, particularly to ensure that the sort is stable.

rschmitt · 2023-04-08T00:55:06Z

httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/InetAddressComparator.java

+        final boolean add1IsReachable;
+        final boolean add2IsReachable;
+        try {
+            add1IsReachable = addr1.isReachable(500);


You can't call this method, especially not from a comparator. The Javadoc states:

* Test whether that address is reachable. Best effort is made by the * implementation to try to reach the host, but firewalls and server * configuration may block requests resulting in an unreachable status * while some specific ports may be accessible. * A typical implementation will use ICMP ECHO REQUESTs if the * privilege can be obtained, otherwise it will try to establish * a TCP connection on port 7 (Echo) of the destination host.

I believe that this is supposed to work by looking at which source addresses are available. For example, if there's no IPv6 source address (for the client's end of the TCP connection), then IPv6 destination addresses are ipso facto unusable.

rschmitt · 2023-04-08T01:02:59Z

httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/InetAddressComparator.java

+                return scope;
+            }
+        }
+        return -1;


Isn't 127.0.0.1/8 supposed to have special treatment?

rschmitt · 2023-04-08T01:05:10Z

...n/java/org/apache/hc/client5/http/impl/nio/HappyEyeballsV2AsyncClientConnectionOperator.java

+                    }
+
+                    // Sort the array of addresses using the custom Comparator
+                    Arrays.sort(inetAddresses, InetAddressComparator.INSTANCE);


Is this a stable sort? It's important to not change the order of IP addresses that are equal under the comparison rules.

arturobernalg · 2023-04-12T23:29:01Z

Hi @rschmitt I've made a commit addressing the feedback you provided on the pull request. I believe I've covered all the points. There isn't a specific JUnit test for the Destination Address Selection (rfc6724) rules; there's only an example test class. Currently, I'm looking into how to approach the testing since the class is internal and not exposed at the moment.
Please feel free to provide any further comments or suggestions.

ok2c · 2023-08-12T15:22:47Z

@rschmitt @rhernandez35 Folks,, is there any chance you could do another review sometime soon? @arturobernalg Please in the meantime change the target branch to 5.4.x.

This commit adds the implementation of HappyEyeballsV2AsyncClientConnectionOperator, a new class that supports asynchronous connection establishment over both IPv4 and IPv6. The operator follows the Happy Eyeballs v2 algorithm to attempt to connect first over IPv6 and then fall back to IPv4 if needed.

ok2c · 2023-12-13T11:22:02Z

@rschmitt @rhernandez35 Is this feature still relevant? What shall we do with this PR? @arturobernalg Could you please change the target branch back to master so I could delete obsolete 5.4.x?

rschmitt requested changes Apr 4, 2023

View reviewed changes

rhernandez35 reviewed Apr 4, 2023

View reviewed changes

arturobernalg force-pushed the feature/happyEyeballs branch from 3b9d109 to 999429c Compare April 7, 2023 21:33

arturobernalg requested a review from rschmitt April 7, 2023 21:38

rschmitt requested changes Apr 8, 2023

View reviewed changes

arturobernalg force-pushed the feature/happyEyeballs branch 6 times, most recently from df3bd5d to 833fc54 Compare April 12, 2023 23:28

arturobernalg force-pushed the feature/happyEyeballs branch from fe6b360 to 25e1168 Compare April 13, 2023 06:34

arturobernalg requested review from rhernandez35 and rschmitt April 18, 2023 22:00

arturobernalg force-pushed the feature/happyEyeballs branch 3 times, most recently from 8283e01 to f0b1668 Compare April 26, 2023 21:34

ok2c force-pushed the 5.3.x branch from 6ffe9b2 to 5505e84 Compare April 27, 2023 15:34

arturobernalg force-pushed the feature/happyEyeballs branch 2 times, most recently from 72a14d7 to 0415171 Compare May 3, 2023 20:08

arturobernalg force-pushed the feature/happyEyeballs branch from f011c40 to 8d8aad0 Compare May 6, 2023 21:42

arturobernalg force-pushed the feature/happyEyeballs branch from 8d8aad0 to 8a60365 Compare June 23, 2023 20:38

arturobernalg force-pushed the feature/happyEyeballs branch from 8a60365 to 2a75ae2 Compare July 25, 2023 16:16

arturobernalg changed the base branch from 5.3.x to 5.4.x August 12, 2023 17:00

ok2c force-pushed the 5.4.x branch from f61fb51 to 046f345 Compare August 20, 2023 09:42

arturobernalg force-pushed the feature/happyEyeballs branch from 2a75ae2 to 89e3d85 Compare August 25, 2023 05:28

ok2c deleted the branch apache:5.4.x December 19, 2023 09:38

ok2c closed this Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement HappyEyeballsV2AsyncClientConnectionOperator #428

Implement HappyEyeballsV2AsyncClientConnectionOperator #428

arturobernalg commented Mar 24, 2023 •

edited

Loading

ok2c commented Mar 27, 2023

ok2c commented Apr 3, 2023

rschmitt Apr 4, 2023

rschmitt Apr 4, 2023

arturobernalg Apr 4, 2023 •

edited

Loading

rschmitt Apr 4, 2023

rhernandez35 Apr 4, 2023

rschmitt Apr 4, 2023

arturobernalg Apr 4, 2023 •

edited

Loading

rschmitt Apr 4, 2023

rschmitt Apr 4, 2023

rschmitt Apr 4, 2023

rschmitt Apr 4, 2023

rhernandez35 Apr 4, 2023

rhernandez35 Apr 4, 2023

arturobernalg commented Apr 7, 2023 •

edited

Loading

rschmitt left a comment

rschmitt Apr 8, 2023

rschmitt Apr 8, 2023

rschmitt Apr 8, 2023

arturobernalg commented Apr 12, 2023 •

edited

Loading

ok2c commented Aug 12, 2023

ok2c commented Dec 13, 2023


		}

		@DisplayName("Test that application prioritizes IPv6 over IPv4 when both are available")

Implement HappyEyeballsV2AsyncClientConnectionOperator #428

Implement HappyEyeballsV2AsyncClientConnectionOperator #428

Conversation

arturobernalg commented Mar 24, 2023 • edited Loading

ok2c commented Mar 27, 2023

ok2c commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturobernalg Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturobernalg Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturobernalg commented Apr 7, 2023 • edited Loading

rschmitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturobernalg commented Apr 12, 2023 • edited Loading

ok2c commented Aug 12, 2023

ok2c commented Dec 13, 2023

arturobernalg commented Mar 24, 2023 •

edited

Loading

arturobernalg Apr 4, 2023 •

edited

Loading

arturobernalg Apr 4, 2023 •

edited

Loading

arturobernalg commented Apr 7, 2023 •

edited

Loading

arturobernalg commented Apr 12, 2023 •

edited

Loading