Provide a way to adapt CompletableFuture into AsyncMethodCallback and vice versa

[trustin]

#112

• Provide a method that returns a new AsyncMethodCallback who updates the state of a specified CompletableFuture when its onComplete() or onError() is called.
• Provide a method that returns a new CompletableFuture who updates the state of a specified AsyncMethodCallback when its complete(), completeExceptionally() or cancel() is called.

e.g.

client.hello("World", FutureConversions.toAsyncMethodCallback(myCompletableFuture));

public void hello(String name, AsyncMethodCallback cb) throws TException {
    otherClient.getGreetings(name, FutureConversions.toCompletableFuture(cb));
}

We might want to add similar methods for Netty promises/futures.

[anuraaga]

I think the callers code would look closer to this

ThriftCallbackFuture<HelloResponse> future = new ThriftCallbackFuture<>();
client.hello("World", future);
return future;

public void hello(String name, AsyncMethodCallback cb) throws TException {
    CompletableFuture<GreetingsResponse> future = new ThriftCallbackFuture<>();
    otherClient.getGreetings(name, future);
    FutureConversions.setFutureResult(future, cb);
}

For reference in case they help, I've written these in server code for adapting with ListenableFuture and use them all the time

/**
 * A listenable future that can be passed in as an {@link AsyncMethodCallback}
 * when making an asynchronous thrift client rpc.
 */
public class ThriftCallbackListenableFuture<T> extends AbstractFuture<T> implements AsyncMethodCallback<T> {

    @Override
    public void onComplete(T t) {
        set(t);
    }

    @Override
    public void onError(Exception e) {
        setException(e);
    }
}

private static <T> void setAsyncResult(AsyncMethodCallback<T> resultHandler, ListenableFuture<T> future) {
        FuturesExtra.addCallback(future, resultHandler::onComplete, t -> {
            logger.warn("Failed to handle request:{}", t, t);
            MyException thrown;
            if (t instanceof MyException) {
                thrown = (MyException) t;
            } else if (t.getCause() instanceof MyException) {
                thrown = (MyException) t.getCause();
            } else if (t instanceof IllegalArgumentException) {
                thrown = new MyException().setCode(MyErrorCode.ILLEGAL_ARGUMENT).setReason(t.getMessage());
            } else {
                thrown = new MyException().setCode(MyErrorCode.INTERNAL_SERVER_ERROR);
                thrown.initCause(t);
            }
            resultHandler.onError(thrown);
        });
    }

I guess translating unknown exceptions to thrift exceptions may be relatively normal and make writing setAsyncResult generically more difficult.

[GrapeBaBa]

From Finagle experience, the hello method is better like
CompletableFuture<HelloResponse> hello(String name) throws TException
caller can use CompletableFuture to composite multiple invoke.
Does it make sense in armeria use cases? Is it difficult to implement?

[trustin]

CompletableFuture<HelloResponse> future = new ThriftCallbackFuture<>();
client.hello("World", future);
return future;

@anuraaga How can this be done? client.hello() requires an AsyncMethodCallback.

From Finagle experience, the hello method is better like
CompletableFuture<HelloResponse> hello(String name) throws TException
caller can use CompletableFuture to composite multiple invoke.
Does it make sense in armeria use cases? Is it difficult to implement?

@GrapeBaBa As mentioned in #112, that would require us to write a code generator. This issue is not about fixing Apache Thrift compiler but providing some adaptation utility until we have a great Thrift compiler.

[anuraaga]

@trustin Sorry, my example does have a slight typo, indeed future has to be declared as ThriftCallbackFuture (fixed my original example code). But then it works since ThriftCallbackFuture implements AsyncMethodCallback but extends CompletableFuture - it can be passed to client.hello() just fine but it's future whenComplete etc methods will also work since we pass the data from thrift's callbacks into the future's completion methods.

[imasahiro]

I think #347 solves this issue. WDYT @trustin ?

[trustin]

Right. Closing. :-)