investigate async http client integration

[codefromthecrypt]

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

Future<Integer> f = asyncHttpClient.prepareGet("http://www.ning.com/").execute(
   new AsyncCompletionHandler<Integer>(){

    @Override
    public Integer onCompleted(Response response) throws Exception{
        // Do something with the Response
        return response.getStatusCode();
    }

    @Override
    public void onThrowable(Throwable t){
        // Something wrong happened.
    }
});

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.

interface Client {
  // existing
  Response execute(Request request, Options options) throws IOException;
  // new
  void execute(Request request, Options options, IncrementalCallback<Response> responseCallback);

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

void execute(Request request, Options options, IncrementalCallback<Response> responseCallback) {
      httpExecutor.get().execute(new Runnable() {
        @Override public void run() {
          Error error = null;
          try {
            responseCallback.onNext(execute(request, options));
            responseCallback.onSuccess();
          } catch (Error cause) {
            // assign to a variable in case .onFailure throws a RTE
            error = cause;
            responseCallback.onFailure(cause);
          } catch (Throwable cause) {
            responseCallback.onFailure(cause);
          } finally {
            Thread.currentThread().setName(IDLE_THREAD_NAME);
            if (error != null)
              throw error;
          }
        }
      }
  }

[codefromthecrypt]

cc @benjchristensen

[benjchristensen]

That makes sense, with IncrementalCallback we can support both blocking and non-blocking IO.

[benjchristensen]

Is there a reason to keep the synchronous method?

// existing
  Response execute(Request request, Options options) throws IOException;

[codefromthecrypt]

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.

[codefromthecrypt]

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.

[aansel]

Any update on this? Are you still planning to integrate this in a future release?
Thanks.

[kflorence]

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.

[codefromthecrypt]

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?

[kflorence]

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

[rstml]

+1. It's quite strange that popular general purpose HTTP client doesn't support async.

[codefromthecrypt]

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

[rstml]

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.

[codefromthecrypt]

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.