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?

Comment From: pivotal-issuemaster

@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.

Comment From: pivotal-issuemaster

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

Comment From: poutsma

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

Comment From: poutsma

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.

Comment From: martin-tarjanyi

  • 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: https://github.com/spring-projects/spring-framework/pull/24700/commits/df32d7f026941fff2dc376d62cceb3424cd34d1f#diff-e25ee214b9bfab881247f141b0ddb407R629-R632
  • Second test case verified response cookie handling: https://github.com/spring-projects/spring-framework/pull/24700/commits/14a63dbf76cce125194d05820246aae3f61501b1#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.

Comment From: poutsma

  • Added the requested code styles changes.

Great! Thanks.

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.

Comment From: martin-tarjanyi

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

Comment From: poutsma

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?

Comment From: martin-tarjanyi

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.

Comment From: rschmitt

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.

Comment From: poutsma

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