HTTP client refactor #11182

yawkat · 2024-09-13T14:49:33Z

This is the continuation of #11158 and #11177.

Now that the previous PRs have migrated the reading/writing to ByteBody, it is time to unify the code paths. This PR is a complex refactor of DefaultHttpClient that merges most of the code for normal exchange requests, streaming requests, and proxy requests into a single sendRequestWithRedirects method. In particular, logging, filtering, connection management, redirect handling and error handling are now merged using the ByteBody API.

This PR is a result of various small refactors that all preserve behavior, but the combined change is large enough that it basically looks like a rewrite. Behavior should be almost the same, and all tests still pass. There are some minor changes I can think of, but they seem positive:

For exchange requests, the configured request-timeout is only applied to the "top" request instead of all nested redirect requests as well. The inner timeouts are pointless anyway, since the outer timeout will trigger before the inner timeouts do.
There was some confusion in the code between the "parent request" (i.e. the request from the ServerRequestContext) and the "original request" (the request that produced a redirect) when it comes to redirect handling, in particular for streamed requests. I think this was simply a bug that was never hit by the test cases because they mostly hit the non-streaming client.
Client filters should be a bit more powerful now. Before this PR, URI resolution (i.e. when you do GET("/foo") it would resolve to GET("https://example.com/foo") depending on service ID) would happen before filters, and then would be carried next to the request through the filters. This means that filters can (a) not replace the request because provideResponse would ignore the new request, and (b) they could not mutate the URI of the original request because the new URI would be ignored in favor of the previously resolved URI. In the new implementation, the URI is still resolved before the filters, but it's set on the request, sent through the filters, and then the filtered request is used in the downstream code. So it should be possible to change the URI in a filter, but I've not added a test.

There are still some more changes I want to make to the client, e.g. getting rid of StreamingHttpRequest for the proxy client. I'm also thinking of exposing sendRawRequest as a new API (another HttpClient interface). But these will happen in future PRs.

This is an intermediate PR of my refactor, in the interest of keeping the changes reviewable.

First version of replacing the previous mixed response handling with a single ByteBody-based handler (similar to PipeliningServerHandler on the server). The purpose of this is to (a) unify some code between the streaming and aggregating code paths in DefaultHttpClient, (b) allow ByteBody-based streaming for ProxyHttpClient in the future so that ProxyHttpClient can be implemented for the jdk client and servlet server, (c) eventually create a ByteBody-based HTTP client API (maybe just in ConnectionManager) that allows lower level access to the request and thus can be used to implement the oci sdk http client.

# Conflicts: # http-netty/src/main/java/io/micronaut/http/netty/body/StreamingNettyByteBody.java

yawkat · 2024-09-13T14:51:24Z

FYI @dstepanov, this PR also migrates the client to use mostly Mono, so this is 90% of the way to ExecutionFlow already. I may do that in a future PR, it just needs a solution to the cancellation feature.

# Conflicts: # http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

graemerocher · 2024-09-19T11:32:34Z

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

+                }
+            }
+
+            pipeline.addLast(ChannelPipelineCustomizer.HANDLER_MICRONAUT_HTTP_RESPONSE, new Http1ResponseHandler(new Http1ResponseHandler.ResponseListener() {


can this be pulled into an inner class to make it easier to read

id prefer not to, it's only like 50 lines but it would require 7 constructor parameters from the method

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

graemerocher · 2024-09-19T11:38:02Z

http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java

@@ -627,8 +627,8 @@ public Optional<io.netty.handler.codec.http.HttpRequest> toHttpRequestDirect() {
    }

    @Override
-    public @NonNull Optional<ByteBody> byteBodyDirect() {
-        return Optional.of(byteBody());
+    public ByteBody byteBodyDirect() {


is this now Nullable?

yes, it's marked as such on the super method

http-client/src/main/java/io/micronaut/http/client/netty/StreamWriter.java

# Conflicts: # http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

http-client/src/main/java/io/micronaut/http/client/netty/NettyClientByteBodyResponse.java

http-client/src/main/java/io/micronaut/http/client/netty/StreamWriter.java

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

dstepanov · 2024-09-20T14:01:08Z

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

+        if (parentRequest != null) {
+            // todo: migrate to new filter
+            filters.add(
+                GenericHttpFilter.createLegacyFilter(new ClientServerContextFilter(parentRequest), new FilterOrder.Fixed(Ordered.HIGHEST_PRECEDENCE))


Do we still need this?

i don't see why not

dstepanov · 2024-09-20T14:04:24Z

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

+            parentRequest,
+            null,
+            request.uri(requestURI),
+            (req, resp) -> {


Please extract a new method

dstepanov · 2024-09-20T14:06:32Z

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

        }
+        // first: connect
+        return connectionManager.connect(requestKey, blockHint)
+            .flatMap(poolHandle -> {


Please extract a new methoda

i dont agree, this code is basically imperative

dstepanov · 2024-09-20T14:12:53Z

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

+                // send the raw request
+                return sendRawRequest(poolHandle, request, byteBody, c -> handleResponseError(request, c));
+            })
+            .flatMap(byteBodyResponse -> {


Please extract a new method

dstepanov · 2024-09-20T14:13:50Z

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

-        ).asNativeBuffer();
-    }
+                // send the raw request
+                return sendRawRequest(poolHandle, request, byteBody, c -> handleResponseError(request, c));


More correct would be to flatMap response of this method instead of the outer one to avoid an extra call for the error path

it's functionally identical

dstepanov · 2024-09-20T14:18:54Z

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

+                    sink.error(e);
+                });
+                pipeline.addLast(streamWriter);
+                byteBuf = null;


Intellij noticed that byteBuf can be null but the Netty API doesn't allow it to be null

all uses of byteBuf are guarded with checks of either byteBuf or streamWriter

dstepanov · 2024-09-20T14:21:24Z

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

+                ).map(r -> (HttpResponse<O>) r);
+            });
+
+        Duration requestTimeout = configuration.getRequestTimeout();


Please extract applyConfiguration

dstepanov · 2024-09-27T08:54:33Z

Some of the code looks way too long, and the code is going beyond my screen. I prefer to extract some of the lamdas to have an appropriate name to make the code more understandable.
Sometimes, there are two different requests, which makes the code confusing.

yawkat · 2024-09-30T10:07:00Z

ive extracted the response handling for exchange. For the other four cases, here is my reasoning why I don't want to:

The buildStreamExchange and proxy methods are thin wrappers around sendRequestWithRedirects, essentially in the form proxy() { return sendRequestWithRedirects((req, resp) -> { ... actual code ... }) }. Because they do so little outside the lambda, I think it's more readable to keep the code there, so there's less "indirection" when reading the code. This outweighs the disadvantage of having a large lambda, imo.
sendRequestWithRedirectsNoFilter has two big lambdas, but imo it's still more readable this way. The mono code is essentially acting like an async-await, the flatMap calls wait for the result of the previous lambda and run the next lambda when that result is available. It is essentially imperative, so you can read the method from top to bottom.
sendRawRequest has a giant Http1ResponseHandler.ResponseListener implementation that is not great. However, that anonymous class only has fairly light implementations of the six methods, only a few lines each. However it uses seven variables from the enclosing method, so if this was a separate class, id need 7 fields, 7 constructor args, 7 sets, which makes everything even more unreadable. So while I think the current version isn't great, the alternative seems worse.

sonarcloud · 2024-09-30T10:31:49Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
84.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

yawkat added 13 commits September 4, 2024 17:15

progress

832a0eb

Merge branch 'refs/heads/4.7.x' into client-request-body

d8a5023

Refactor DefaultHttpClient to use ByteBody for the request body

ef5adae

This is an intermediate PR of my refactor, in the interest of keeping the changes reviewable.

Buffer limits for SseSplitter

c61499a

Merge branch 'refs/heads/4.7.x' into client-response-body

4fe48ff

# Conflicts: # http-netty/src/main/java/io/micronaut/http/netty/body/StreamingNettyByteBody.java

Fix test

3fab394

checkstyle

1f4db5b

Merge branch '4.7.x' into client-request-body

e651961

# Conflicts: # http-netty/src/main/java/io/micronaut/http/netty/body/StreamingNettyByteBody.java

Merge branch 'client-request-body' into client-response-body

504caf5

# Conflicts: # http-netty/src/main/java/io/micronaut/http/netty/body/StreamingNettyByteBody.java

SseSplitter docs

77e0a50

progress

12a879d

progress

f66ae73

yawkat added this to the 4.7.0 milestone Sep 13, 2024

yawkat mentioned this pull request Sep 19, 2024

Refactor DefaultHttpClient to use ByteBody for the request body #11158

Merged

Merge branch '4.7.x' into client-response-body

c8cd5dd

# Conflicts: # http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

graemerocher requested changes Sep 19, 2024

View reviewed changes

yawkat added 4 commits September 19, 2024 14:03

review comments

11c7a6d

refactor Http1ResponseHandler

f499939

checkstyle

a34f0be

sonar

421b85c

graemerocher requested a review from dstepanov September 20, 2024 08:50

yawkat added 2 commits September 20, 2024 10:52

additional check in state transition

0a7cb63

Merge branch 'client-response-body' into client-refactor2

f5dbedb

Base automatically changed from client-response-body to 4.7.x September 20, 2024 09:43

yawkat added 2 commits September 20, 2024 11:45

Merge branch 'refs/heads/4.7.x' into client-refactor2

68bfa35

# Conflicts: # http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

review

7c8c370

yawkat marked this pull request as ready for review September 20, 2024 10:03

yawkat added the type: improvement A minor improvement to an existing feature label Sep 20, 2024

yawkat requested a review from graemerocher September 20, 2024 10:17

graemerocher approved these changes Sep 20, 2024

View reviewed changes

dstepanov requested changes Sep 20, 2024

View reviewed changes

review

2fdd1d1

dstepanov approved these changes Sep 30, 2024

View reviewed changes

yawkat merged commit f74dae1 into 4.7.x Sep 30, 2024
21 checks passed

yawkat deleted the client-refactor2 branch September 30, 2024 13:10

yawkat mentioned this pull request Oct 7, 2024

RawHttpClient #11227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP client refactor #11182

HTTP client refactor #11182

yawkat commented Sep 13, 2024

yawkat commented Sep 13, 2024

graemerocher Sep 19, 2024

yawkat Sep 20, 2024

graemerocher Sep 19, 2024

yawkat Sep 20, 2024

dstepanov Sep 20, 2024

yawkat Sep 30, 2024

dstepanov Sep 20, 2024

dstepanov Sep 20, 2024

yawkat Sep 26, 2024

dstepanov Sep 20, 2024

dstepanov Sep 20, 2024

yawkat Sep 26, 2024

dstepanov Sep 20, 2024

yawkat Sep 26, 2024

dstepanov Sep 20, 2024

dstepanov commented Sep 27, 2024

yawkat commented Sep 30, 2024

sonarcloud bot commented Sep 30, 2024

HTTP client refactor #11182

HTTP client refactor #11182

Conversation

yawkat commented Sep 13, 2024

yawkat commented Sep 13, 2024

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

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

Choose a reason for hiding this comment

dstepanov commented Sep 27, 2024

yawkat commented Sep 30, 2024

sonarcloud bot commented Sep 30, 2024

Quality Gate passed