palantir / dialogue

A client-side RPC library for conjure-java
Apache License 2.0
29 stars 16 forks source link

Discuss relaxing API to support NIO in the future #623

Open gatesn opened 4 years ago

gatesn commented 4 years ago

What happened?

Currently the Channel interface returns a ListenableFuture<Response> and a Response object has an InputStream body. This makes it hard (impossible?) to efficiently implement NIO in the future.

What did you want to happen?

Should we break the Response interface now to parameterise the body type? This more closely reflects the Java11 HttpResponse object and would (I think? not yet too familiar with this) allow us to make better use of non-blocking HTTP clients.

e.g. for the Java HTTP client, we could continue to convert back to InputStream (with [BodySubscribers#ofInputStream](https://docs.oracle.com/en/java/javase/11/docs/api/java.net.http/java/net/http/HttpResponse.BodySubscribers.html#ofInputStream())) then perhaps move to using the Jackson non-blocking parser interface in the future.

iamdanfox commented 4 years ago

I like the idea of trying to make dialogue NIO-friendly before we get too locked in.

Reading through the BodyHandlers.ofString(), it seems that the java client passes data by calling a void onNext(List<ByteBuffer> items) method and then finally calling onComplete(). I'm not 100% sure why they pass a List<ByteBuffer> rather than just calling the method multiple times, but there probably a perf optimization somewhere.

One thing to keep in mind though is that it would probably be lame to make our ApacheHttpClient calls (which expose blocking APIs) translate from InputStream -> some async flowable thingy and then translate back -> InputStream.

gatesn commented 4 years ago

We could use the AsyncApacheHttpClient, but I think you're right, we don't want to take a perf hit for blocking http clients.

What would be the next steps, perhaps pushing up a prototype using a non-blocking http client to see where things fall apart?

jkozlowski commented 4 years ago

@iamdanfox likely optimisation to reduce number of syscalls.