Integrate Apache Http client with WebClient #24700

martin-tarjanyi · 2020-03-14T22:32:16Z

This PR aims to integrate Apache HttpClient 5 (which supports reactive streams and became GA last month) with WebClient.

Questions

Does Spring have some internal load tests which can be run the new implementation to see how it behaves under stress?
Should we support some kind of resource sharing just like it's done for the netty and jetty implementations?

pivotal-issuemaster · 2020-03-14T22:34:04Z

@martin-tarjanyi Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

spring-web/src/main/java/org/springframework/http/client/reactive/ApacheClientHttpResponse.java

pivotal-issuemaster · 2020-03-14T22:36:47Z

@martin-tarjanyi Thank you for signing the Contributor License Agreement!

…pache

poutsma · 2020-03-26T09:30:45Z

I will try and pick this up in the 5.3 timeline.

poutsma · 2020-03-26T15:02:35Z

Thank you for submitting this PR. Could you please make sure that it conforms to our Code Style? This makes it significantly easier for us to review the PR.

Specifically, please make sure that there is an empty line in between class declaration and fields, and two before constructors. Please refer to other classes in the Spring Framework to see how they are formatted.

…pache # Conflicts: # spring-webflux/src/test/java/org/springframework/web/reactive/function/client/WebClientIntegrationTests.java

martin-tarjanyi · 2020-03-26T21:00:31Z

Added the requested code styles changes.
Reverted my change in WebClientIntegrationTests as @rstoyanchev made some refactoring there.
- First test verified multiple request cookie handling and showed a potential bug in reactor-netty. This probably should be handled as a separate PR/issue: df32d7f#diff-e25ee214b9bfab881247f141b0ddb407R629-R632
- Second test case verified response cookie handling: 14a63db#diff-e25ee214b9bfab881247f141b0ddb407R650-R684 - I found this test valuable as cookie mapping was a bit trickier in the Apache implementation than in the others. Now, I'm a bit uncertain whether I should extend some exisitng test case with this or just create a new one.
- Please, advise on these.

poutsma · 2020-03-27T13:12:07Z

Added the requested code styles changes.

Great! Thanks.

First test verified multiple request cookie handling and showed a potential bug in reactor-netty. This probably should be handled as a separate PR/issue: df32d7f#diff-e25ee214b9bfab881247f141b0ddb407R629-R632

Indeed, I can duplicate this issue locally. Could you please file a separate issue against Spring Framework for this problem? Not sure if the reason is in our connector or in Reactor Netty itself, but we can migrate the issue if need be.

Second test case verified response cookie handling: 14a63db#diff-e25ee214b9bfab881247f141b0ddb407R650-R684 - I found this test valuable as cookie mapping was a bit trickier in the Apache implementation than in the others. Now, I'm a bit uncertain whether I should extend some exisitng test case with this or just create a new one.

Let's keep this as is, i.e. a new test in WebClientIntegrationTests, for now. It seems like a useful test to have.

I am now going to take a closer look at the code, so review comments will follow.

poutsma

Thanks again for submitting this PR.

While going through the code, I have some comments. Can you please resolve them?

...ng-web/src/main/java/org/springframework/http/client/reactive/ApacheClientHttpConnector.java

spring-web/src/main/java/org/springframework/http/client/reactive/ApacheClientHttpRequest.java

poutsma · 2020-03-27T13:57:57Z

spring-web/src/main/java/org/springframework/http/client/reactive/ApacheClientHttpRequest.java

+
+	@Override
+	public Mono<Void> writeAndFlushWith(Publisher<? extends Publisher<? extends DataBuffer>> body) {
+		return writeWith(Flux.from(body).flatMap(p -> p));


This method is supposed to flush after each inner buffer flux is complete. Is this code doing that?

Likely not, but I'm not sure. Let me tag Apache contributors @ok2c and @rschmitt maybe they can help out here.
So the question is: Is it possible to control when the reactive data producer flushes parts of the request body?
Also, folks, you are more than welcome to review this pull request if you have time.

@martin-tarjanyi I personally have little experience with reactive bindings but I hope @rschmitt can answer your question. What I could do though is to look at various ways of optimizing the implementation once it gets accepted.

spring-web/src/main/java/org/springframework/http/client/reactive/ApacheClientHttpRequest.java

spring-web/src/main/java/org/springframework/http/client/reactive/ApacheClientHttpResponse.java

...ng-web/src/main/java/org/springframework/http/client/reactive/MonoFutureCallbackAdapter.java

spring-web/src/main/java/org/springframework/http/client/reactive/ApacheClientHttpRequest.java

spring-web/src/main/java/org/springframework/http/client/reactive/ApacheClientHttpResponse.java

martin-tarjanyi · 2020-03-29T21:23:04Z

I've addressed most of the comments and answered where I have concerns.

poutsma · 2020-03-30T14:05:43Z

Thank you. I have some further comments and changes I'd like to see, but I do not want to ask too much of you. The code is definitely in a good enough state now for me to take over.

Would you be willing to go through another round of comments? Or should I wrap it up?

martin-tarjanyi · 2020-03-30T15:54:31Z

I'm happy to work on this, but if you think at this point it would be more efficient to take over then feel free to do so. Thanks for your review.

poutsma

Hi and thanks again,

I have left some more comments. Please help me resolve them.

...rc/main/java/org/springframework/http/client/reactive/HttpComponentsClientHttpConnector.java

.../src/main/java/org/springframework/http/client/reactive/HttpComponentsClientHttpRequest.java

rschmitt · 2020-04-10T05:27:23Z

This looks good, from the perspective of httpcore5-reactive. I'm pleased to see that this integrates so cleanly with your existing abstractions (e.g. ReactiveHttpInputMessage); it's pretty much exactly what we were going for.

* webclient_apache: Integrate Apache http client with WebClient

poutsma · 2020-04-29T13:24:38Z

This PR has now been merged. Thanks for contributing, @martin-tarjanyi! And thanks for your support, @ok2c and @rschmitt.

This commit introduces a ClientHttpConnector implementation backed by Apache HttpComponents HttpClient 5.0. Fixes spring-projectsgh-24700

Integrate Apache http client with WebClient

Verified

This commit was signed with the committer’s verified signature.

metacosm Chris Laprun

SSH Key Fingerprint: tBGIjbs0VWiYO/HfcV+SyH+JGUVOxyF6XK9cFGfNTXc
Verified
Learn about vigilant mode

14a63db

spring-projects-issues added the status: waiting-for-triage label Mar 14, 2020

martin-tarjanyi commented Mar 14, 2020

View reviewed changes

spring-web/src/main/java/org/springframework/http/client/reactive/ApacheClientHttpResponse.java Outdated Show resolved Hide resolved

martin-tarjanyi added 4 commits March 14, 2020 23:37

Fix javadoc

1bf43f0

Minor code and doc polishment

85dd3b7

Merge remote-tracking branch 'remotes/origin/master' into webclient_a…

78720fd

…pache

Fix handling of multiple request cookies for apache http client

df32d7f

poutsma self-assigned this Mar 26, 2020

poutsma added this to the 5.3 M1 milestone Mar 26, 2020

Conform to code style

3a7f4ed

rstoyanchev added in: web type: enhancement and removed status: waiting-for-triage labels Mar 26, 2020

Merge remote-tracking branch 'remotes/origin/master' into webclient_a…

7f8e1f2

…pache # Conflicts: # spring-webflux/src/test/java/org/springframework/web/reactive/function/client/WebClientIntegrationTests.java

poutsma reviewed Mar 27, 2020

View reviewed changes

martin-tarjanyi added 6 commits March 27, 2020 23:40

Rename Apache client components for consistency

ac233bb

Polish HttpComponents WebClient implementation

d33dc05

Re-add response cookie integration test for WebClient

1a92f3c

Use CookieStore to send cookies with HttpComponents client

58f8488

Move response body stream handling to getBody method

3b032e2

Pass content encoding and type to ReactiveEntityProducer

b5c2fa0

poutsma reviewed Apr 2, 2020

View reviewed changes

Code review changes

f578f6e

poutsma closed this in 3bc1d42 Apr 29, 2020

poutsma added a commit that referenced this pull request Apr 29, 2020

Merge pull request gh-24700 from martin-tarjanyi:webclient_apache

6b1170b

* webclient_apache: Integrate Apache http client with WebClient

kenny5he pushed a commit to kenny5he/spring-framework that referenced this pull request Jun 21, 2020

Integrate Apache http client with WebClient

c01802a

This commit introduces a ClientHttpConnector implementation backed by Apache HttpComponents HttpClient 5.0. Fixes spring-projectsgh-24700

wilkinsona mentioned this pull request Apr 11, 2021

Provide auto-configuration for using Apache HttpClient 5 with WebClient spring-projects/spring-boot#26004

Closed

bclozel mentioned this pull request Aug 4, 2022

Upgrade RestTemplate to HttpClient 5 #28925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Apache Http client with WebClient #24700

Integrate Apache Http client with WebClient #24700

martin-tarjanyi commented Mar 14, 2020 •

edited

Loading

pivotal-issuemaster commented Mar 14, 2020

pivotal-issuemaster commented Mar 14, 2020

poutsma commented Mar 26, 2020

poutsma commented Mar 26, 2020

martin-tarjanyi commented Mar 26, 2020

poutsma commented Mar 27, 2020 •

edited

Loading

poutsma left a comment

poutsma Mar 27, 2020

martin-tarjanyi Mar 29, 2020

ok2c Mar 30, 2020

martin-tarjanyi commented Mar 29, 2020

poutsma commented Mar 30, 2020

martin-tarjanyi commented Mar 30, 2020

poutsma left a comment

rschmitt commented Apr 10, 2020

poutsma commented Apr 29, 2020

Integrate Apache Http client with WebClient #24700

Integrate Apache Http client with WebClient #24700

Conversation

martin-tarjanyi commented Mar 14, 2020 • edited Loading

Questions

pivotal-issuemaster commented Mar 14, 2020

pivotal-issuemaster commented Mar 14, 2020

poutsma commented Mar 26, 2020

poutsma commented Mar 26, 2020

martin-tarjanyi commented Mar 26, 2020

poutsma commented Mar 27, 2020 • edited Loading

poutsma left a comment

Choose a reason for hiding this comment

poutsma Mar 27, 2020

Choose a reason for hiding this comment

martin-tarjanyi Mar 29, 2020

Choose a reason for hiding this comment

ok2c Mar 30, 2020

Choose a reason for hiding this comment

martin-tarjanyi commented Mar 29, 2020

poutsma commented Mar 30, 2020

martin-tarjanyi commented Mar 30, 2020

poutsma left a comment

Choose a reason for hiding this comment

rschmitt commented Apr 10, 2020

poutsma commented Apr 29, 2020

martin-tarjanyi commented Mar 14, 2020 •

edited

Loading

poutsma commented Mar 27, 2020 •

edited

Loading