Async client for apache http 5

[velo]

@motinis in case you wanna see what I did based on your work

[motinis]

Looks great, thank you! I've put in some suggestions there, mainly around HttpClientContext

[motinis]

Suggested change

	final HttpClientContext context = new HttpClientContext();
	final HttpClientContext context = requestContext.orElseGet(HttpClientContext::new)

Use the client supplied context, else make a new default just for this request.

[kdavisk6]

@velo You have some changes here that aren't about the HC5 client, can we limit this PR to just the HC5 changes?

[velo]

@velo You have some changes here that aren't about the HC5 client, can we limit this PR to just the HC5 changes?

My bad, IDE automatically fixed stuff.

Only change required on feign core was make a few classes public so I could run the same tests on hc5

[motinis]

I think that putting context onto the Request itself is preferable to having an extra parameter. One could indeed to this via Contract; and indeed the common cases could be supported with explicit enums in annotations. I think as well that these common cases should likely be fulfilled by the async client class. I also think that there does need to be an ability for the user to specify via Contract or Builder an explicit context provider for use-cases which wouldn't make sense to put into Feign.

…

On Fri, Feb 28, 2020 at 2:35 AM Kevin Davis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In hc5/src/main/java/feign/hc5/AsyncApacheHttp5Client.java <#1179 (comment)>: > +import java.util.*; +import java.util.concurrent.CompletableFuture; +import feign.*; +import feign.Request.Options; + +/** + * This module directs Feign's http requests to Apache's + * <a href="https://hc.apache.org/httpcomponents-client-5.0.x/index.html">HttpClient 5</a>. Ex. + * + * <pre> + * GitHub github = Feign.builder().client(new ApacheHttp5Client()).target(GitHub.class, + * "https://api.github.com"); + */ +/* + */ +public final class AsyncApacheHttp5Client<C> implements AsyncClient<C> { I'm not convinced we need to add another construct like the Context when we have Request already, which represents the canonical information required for a Client to execute a request. It's already thread-safe and immutable. We should consider extending Request before adding another concept. Another counter point, this Context provides users a way to circumvent our internal separation and, if history is any indication, will become a cause for confusion. If you decide to continue to use this Context, I strongly suggest that we make it part of our private API usage. Cookies and Session management can be something that we should support first at the Contract level, ensuring compatibility throughout the ecosystem and preserve the domain separation Feign is good at, which we should address separately from this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1179?email_source=notifications&email_token=AEAZAM5GI3FX7FHOXEGW75LRFBL5HA5CNFSM4K4T7HO2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXJCSIY#discussion_r385449575>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEAZAM63HL2XBUICCBZ57HLRFBL5HANCNFSM4K4T7HOQ> .