Open codefromthecrypt opened 11 years ago
cc @benjchristensen
That makes sense, with IncrementalCallback
we can support both blocking and non-blocking IO.
Is there a reason to keep the synchronous method?
// existing
Response execute(Request request, Options options) throws IOException;
yes because most api interactions with http are still synchronous and we don't want to or need to eliminate "easy mode" just to facilitate advanced.
for example, implementing Client with url connection (a synchronous client) could reuse a base implementation of forking off the request. I've in the past made the mistake of forcing synchronous http calls through blocking futures (jclouds), and don't want to repeat that as a default, particularly as it would require sync calls to unnecessarily fire threads.
Having an interface that supports both means allows folks to decide at configuration time how they prefer to interact, whether that's making everything block on futures, use real async tasks or use async only when asked.
Any update on this? Are you still planning to integrate this in a future release? Thanks.
Please implement support for async, it's a pretty big drawback right now when your client and MVC controller can't share the same contract because of the use of (for example) DeferredResult
.
DeferredResult is a spring MVC type, right? If that's the goal, maybe raise an issue in spring-cloud-netflix where the MVC contract lives?
@adriancole Yes, it is a spring MVC type, however, if the underlying implementation doesn't support async I don't see how spring-cloud-netflix
would be able to solve the problem. I will take your suggestion and raise an issue there, though, in the hopes that maybe there is a workaround. Thanks for the quick response!
+1. It's quite strange that popular general purpose HTTP client doesn't support async.
This issue seems to be mixed up with Spring MVC types which is not here, rather in spring-cloud-netflix.
Currently, the path to async is feign-hystrix, which allows you to get a number of asynchronous types back ex. (future, rx).
I see. Actually planning to use Hystrix w/ Rx. However what concerns me is the lack of non-blocking client support. Maybe I'm missing something, but it seems that Feign uses either Apache HttpClient or OkHttp - both in blocking mode? Was hoping that Async HttpClient would allow me to do non-blocking requests.
Yeah I think we should have a separate topic for non-blocking vs async invocation. In my mind, I thought something like rxnetty could be a faster path towards this, since for example, the callbacks are built to work together with the non-blocking transport.
Significant surgery is needed regardless, as Feign't client type is blocking. I'd rather special-case one combo (I think we talked about "stacks" before) vs design a base client which seems to be portable, but actually only works reasonably with one http transport and one invocation/composition model.
Per a discussion with @niteshkant, I believe we can make a compatible http response callback interface to facilitate nio support.
Context
Many async http clients provide a callback interface such as below
https://github.com/AsyncHttpClient/async-http-client
How
It should be possible to extend our
Client
to be compatible with the callback system from one or more async http clients, avoiding the need to independently manage threads in Feign.ex.
An asynchronous client would need to map their callback to ours.
Any synchronous http client (including our default) could extend from a base class that implements the callback method like so