[ 
https://issues.apache.org/jira/browse/THRIFT-3112?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15137318#comment-15137318
 ] 

John Sirois commented on THRIFT-3112:
-------------------------------------

The fundamental problem here is that the async client stack is parametrized 
over its implementation.  An example from the tutorial "correctly" parametrized 
today:
{code:java}
package tutorial;

public class Calculator {
  public interface AsyncIface extends shared.SharedService .AsyncIface {
    public void 
ping(org.apache.thrift.async.AsyncMethodCallback<AsyncClient.ping_call> 
resultHandler) throws org.apache.thrift.TException;
    public void add(int num1, int num2, 
org.apache.thrift.async.AsyncMethodCallback<AsyncClient.add_call> 
resultHandler) throws org.apache.thrift.TException;
    ...
  }
  public static class AsyncClient extends shared.SharedService.AsyncClient 
implements AsyncIface {
    public void ping(org.apache.thrift.async.AsyncMethodCallback<ping_call> 
resultHandler) throws org.apache.thrift.TException {
      ...
    }
    public static class ping_call extends 
org.apache.thrift.async.TAsyncMethodCall<ping_call> {
      ...
      public void getResult() throws org.apache.thrift.TException {
        ...
      }
    }
    ...
  }
  public static class AsyncProcessor<I extends AsyncIface> extends 
shared.SharedService.AsyncProcessor<I> {
    public static class ping<I extends AsyncIface> extends 
org.apache.thrift.AsyncProcessFunction<I, ping_args, Void> {
      public AsyncMethodCallback<Void> getResultHandler(final AsyncFrameBuffer 
fb, final int seqid) {
        ...
      }
    }
    public static class add<I extends AsyncIface> extends 
org.apache.thrift.AsyncProcessFunction<I, add_args, Integer> {
      public AsyncMethodCallback<Integer> getResultHandler(final 
AsyncFrameBuffer fb, final int seqid) {
        ...
      }
    }
    ...
  }
}  
{code}

Notice in the {{AsyncIface}} that:
# The client-side AsyncMethodCallback's are not parametrized over the sync 
interface method return type as you'd expect, they are parametrized by the 
client implementation of the method call.
# The server-side AsyncMethodCallback's are parametrized as you'd expect - over 
the sync interface method return type.

The combination of 1 and 2 is confusing, but worse 1 mixes the implementation - 
{{AsyncClient}} - into the interface - {{AsyncIface}} and it forces clients to 
both call {{getResult}} in their {{onComplete}} handler as well as deal with 
errors.  This leaves error handling split between {{onComplete}} and 
{{onError}}.

It seems to me the right long-term answer is to fix the generated client code 
to match the generated server code and pass the successfull completed result to 
{{onComplete}} and not the {{TAsyncMethodCall}} implementation.  This would be 
a breaking change though and anyone using AsyncIface today would need to fix 
there code on upgrade.  A [github 
search|https://github.com/search?utf8=%E2%9C%93&q=new+AsyncMethodCallback+language%3AJava+extension%3Ajava&type=Code&ref=advsearch&l=Java&l=]
 does reveal folks actually using {{AsyncIface}} today.  That said - in the 1st 
page of results alone there is evidence of folks doing it "wrong" and 
parametrizing in the natural way.  That code must either be unused or else 
written against an Apache Thrift fork.

> [Java] AsyncMethodCallback should be typed in generated AsyncIface
> ------------------------------------------------------------------
>
>                 Key: THRIFT-3112
>                 URL: https://issues.apache.org/jira/browse/THRIFT-3112
>             Project: Thrift
>          Issue Type: Improvement
>          Components: Java - Compiler
>    Affects Versions: 0.9.2
>            Reporter: Sergei Egorov
>            Assignee: John Sirois
>              Labels: easyfix
>
> AsyncMethodCallback is generic, but current Java code generator is not adding 
> type info to it, and instead of:
> {code:java}
> public interface AsyncIface {
>     public void ping(org.apache.thrift.async.AsyncMethodCallback<String> 
> resultHandler) throws org.apache.thrift.TException;
>  }
> {code}
> we have:
> {code:java}
> public interface AsyncIface {
>     public void ping(org.apache.thrift.async.AsyncMethodCallback 
> resultHandler) throws org.apache.thrift.TException;
>  }
> {code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to