Simple Client Interceptor

[Sivabalan]

Hey folks,

     I am looking to add grpc support to mobile networking stack at my workplace and was wondering if we can simplify the clientInterceptor interface. I do understand the reasons for it being a little complicated, but we could come up with a simpler one. I am by no means an expert in grpc, but wanted to see if we can simplify efforts required by the devs. 

This is not a breaking change, my proposal is to introduce a simpler abstract class which devs can choose to extend rather than implementing ClientInterceptor. I come from java land and so the proposal is in java, but should be generalizable. 

public abstract class SimpleClientInterceptor<ReqT, RespT> { /** * Invoked with outgoing request headers. * @param requestHeaders request headers that are sent as part of the request. */ protected abstract void onRequestHeaders(Metadata requestHeaders); /** * Invoked when a message is sent out from client to server. * @param message message being sent out. */ protected abstract void onRequestMessage(ReqT message); /** * Invoked with response headers. * @param responseHeaders response headers that are received from the server. */ protected abstract void onResponseHeaders(Metadata responseHeaders); /** * Invoked with message received from the server. * @param msg message being received. */ protected abstract void onResponseMessage(RespT msg); /** * Invoked when the call is closed. * @param status * @param trailers */ protected void onClose(Status status, Metadata trailers) {} }

Here is the impl draft on how we could achieve this. Basic idea is to add a ClientCallHandler/wrapper and call into the interceptor impl where ever required. 

As of today, an interceptor which just processes headers looks complicated as below. 

public class HeaderClientInterceptor extends InterceptorChannel {   public HeaderClientInterceptor(InterceptorChannel next){       super(next);   }   @Override   public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(MethodDescriptor<ReqT, RespT> method,       CallOptions callOptions) {       return new SimpleForwardingClientCall<ReqT, RespT>(next.newCall(method, callOptions)) {           @Override           public void start(Listener<RespT> responseListener, Metadata headers) {               // process request headers. add/edit/remove headers.               super.start(new SimpleForwardingClientCallListener<RespT>(responseListener) {                   @Override                   public void onHeaders(Metadata headers) {                       super.onHeaders(headers);                   }               }, headers);           }       };   }   @Override   public String authority() {       return next.authority();   } }

Definitely, this looks complicated just to process/edit request headers and demands a learning curve for someone who is looking to implement an interceptor. 

With the new proposal, an interceptor which accesses some headers would be like below.

public class HeaderInterceptor<ReqT, RespT> extends SimpleClientInterceptor<ReqT, RespT> { public HeaderInterceptor(InterceptorChannel next) {   super(next); } @Override protected void onRequestHeaders(Metadata requestHeaders) {     // process request headers. add/ edit / remove headers   } @Override protected void onRequestMessage(ReqT message) {   } @Override protected void onResponseHeaders(Metadata responseHeaders) {     // process response headers. add/edit/remove } @Override protected void onResponseMessage(RespT msg) { } }

Implementors don't need to worry about calling next entity in the chain as its taken care by the CallHandler/wrapper. More details can be checked out here on how to get this done. 

Just a reminder, that this is not a breaking change, just that a new abstract class will be introduced which devs can use if they want to, instead of the ClientInterceptor interface directly. 

Would love to hear your thoughts. 

--

Regards,
-Sivabalan

[Eric Anderson]

On Thu, Jul 16, 2020 at 9:32 AM Sivabalan <n.si...@gmail.com> wrote:

This is not a breaking change, my proposal is to introduce a simpler abstract class which devs can choose to extend rather than implementing ClientInterceptor.

ClientInterceptors are really just a small tweak to a Channel to allow them to be plugged together by passing Channel next. Your SimpleClientInterceptor should implement ClientInterceptor instead of InterceptorChannel and then it'd be a drop-in option anywhere ClientInterceptor is available today.

You shouldn't generally make interceptors implement ManagedChannel because then it is hard to determine which channel should be shut down, and when it is shut down which other channels it will shut down.

I come from java land and so the proposal is in java, but should be generalizable. 

public abstract class SimpleClientInterceptor<ReqT, RespT> { /**

* Invoked when a message is sent out from client to server. * @param message message being sent out. */ protected abstract void onRequestMessage(ReqT message);

This mainly supports "observing" client interceptors, that don't need to make any changes to the RPC other than Metadata. That helps many, but lots of interceptors need more power than this. It's still fine to have a simpler version in certain scenarios.

Here is the impl draft on how we could achieve this. Basic idea is to add a ClientCallHandler/wrapper and call into the interceptor impl where ever required. 

As I said above, SimpleClientInterceptor should implement ClientInterceptor instead of InterceptorChannel. ClientCallHandler would be simpler/better if it extended SimpleFowardingClientCall. Your InternalListener would break flow control currently, as it does not forward onReady. It would be better to extend SimpleForwardingClientCallListener.

As of today, an interceptor which just processes headers looks complicated as below. 

// process request headers. add/edit/remove headers.               super.start(new SimpleForwardingClientCallListener<RespT>(responseListener) {                   @Override                   public void onHeaders(Metadata headers) {                       super.onHeaders(headers);                   }               }, headers);

...

@Override   public String authority() {       return next.authority();   } }

authority() is not part of the interceptor API, so that would not be present. Also, since it is not interested in response headers (only request headers), it can use super.start(responseListener, headers); and avoid the SimpleForwardingClientCallListener usage.

That's actually an important point: many interceptors don't need response information, so they don't need to wrap the Listener. That is a benefit as less indirection occurs for events.

I do think it was a mistake to have Metadata passed to start() instead of Channel.newCall()/ClientInterceptor.interceptCall(). That would have simplified things for interceptors that only need the request Metadata. It was less clear in the day when the decision was made, because of some tradeoffs.

With the new proposal, an interceptor which accesses some headers would be like below.

@Override protected void onRequestMessage(ReqT message) {   }

I suggest you have default noop implementations for all the methods. That way implementations don't need to override all methods.

Implementors don't need to worry about calling next entity in the chain as its taken care by the CallHandler/wrapper. More details can be checked out here on how to get this done. 

Just a reminder, that this is not a breaking change, just that a new abstract class will be introduced which devs can use if they want to, instead of the ClientInterceptor interface directly. 

I think this is a good and great thing for you to have and share with others. I wouldn't be as excited for it to be in io.grpc itself, though, as many interceptors can't be implemented using it. That causes a burden because users then need to convert their interceptor to a different API when they need certain control, and it wouldn't generally be obvious to them that the API they are using doesn't support whatever specific thing they need at the time.

If it just had Metadata, especially request metadata, then its limits would be more obvious. But users frequently ask how they fail a call, and you can't fail a call with this type of API. I'm not sure users would realize the API has that limitation. But with some documentation maybe that could be addressed.

[Sivabalan]

Thanks for the detailed response Eric. Appreciate it. Find my responses inline. 

On Mon, Jul 20, 2020 at 11:02 AM Eric Anderson <ej...@google.com> wrote:

On Thu, Jul 16, 2020 at 9:32 AM Sivabalan <n.si...@gmail.com> wrote:

This is not a breaking change, my proposal is to introduce a simpler abstract class which devs can choose to extend rather than implementing ClientInterceptor.

ClientInterceptors are really just a small tweak to a Channel to allow them to be plugged together by passing Channel next. Your SimpleClientInterceptor should implement ClientInterceptor instead of InterceptorChannel and then it'd be a drop-in option anywhere ClientInterceptor is available today.

      - My bad. You are right. Implementing ClientInterceptor would suffice in my proposal.

 

You shouldn't generally make interceptors implement ManagedChannel because then it is hard to determine which channel should be shut down, and when it is shut down which other channels it will shut down.

          - Gotcha.

I come from java land and so the proposal is in java, but should be generalizable. 

public abstract class SimpleClientInterceptor<ReqT, RespT> { /** * Invoked when a message is sent out from client to server. * @param message message being sent out. */ protected abstract void onRequestMessage(ReqT message);

This mainly supports "observing" client interceptors, that don't need to make any changes to the RPC other than Metadata. That helps many, but lots of interceptors need more power than this. It's still fine to have a simpler version in certain scenarios.

        thanks. Can. you throw some light on what more power you are insinuating.  

Here is the impl draft on how we could achieve this. Basic idea is to add a ClientCallHandler/wrapper and call into the interceptor impl where ever required. 

As I said above, SimpleClientInterceptor should implement ClientInterceptor instead of InterceptorChannel. ClientCallHandler would be simpler/better if it extended SimpleFowardingClientCall. Your InternalListener would break flow control currently, as it does not forward onReady. It would be better to extend SimpleForwardingClientCallListener.

      - My impl is just a draft and I missed to add onReady(). I will checkout SimpleForwardingClientCallListener.

 

As of today, an interceptor which just processes headers looks complicated as below. 

// process request headers. add/edit/remove headers.               super.start(new SimpleForwardingClientCallListener<RespT>(responseListener) {                   @Override                   public void onHeaders(Metadata headers) {                       super.onHeaders(headers);                   }               }, headers); ...   @Override   public String authority() {       return next.authority();   } }

authority() is not part of the interceptor API, so that would not be present. Also, since it is not interested in response headers (only request headers), it can use super.start(responseListener, headers); and avoid the SimpleForwardingClientCallListener usage.

    - got it, thanks.

That's actually an important point: many interceptors don't need response information, so they don't need to wrap the Listener. That is a benefit as less indirection occurs for events.

    - Yeah, I see that. not all interceptors are interested in response info.  

I do think it was a mistake to have Metadata passed to start() instead of Channel.newCall()/ClientInterceptor.interceptCall(). That would have simplified things for interceptors that only need the request Metadata. It was less clear in the day when the decision was made, because of some tradeoffs.

    - Yeah, even I had the same question when I was reading and trying to understand grpc, as to why start() has to take in the headers rather than initialization.

With the new proposal, an interceptor which accesses some headers would be like below.

@Override protected void onRequestMessage(ReqT message) {   }

I suggest you have default noop implementations for all the methods. That way implementations don't need to override all methods.

     - Yes, I realized that after posting it to the group.  

Implementors don't need to worry about calling next entity in the chain as its taken care by the CallHandler/wrapper. More details can be checked out here on how to get this done. 

Just a reminder, that this is not a breaking change, just that a new abstract class will be introduced which devs can use if they want to, instead of the ClientInterceptor interface directly. 

I think this is a good and great thing for you to have and share with others. I wouldn't be as excited for it to be in io.grpc itself, though, as many interceptors can't be implemented using it. That causes a burden because users then need to convert their interceptor to a different API when they need certain control, and it wouldn't generally be obvious to them that the API they are using doesn't support whatever specific thing they need at the time.

If it just had Metadata, especially request metadata, then its limits would be more obvious. But users frequently ask how they fail a call, and you can't fail a call with this type of API. I'm not sure users would realize the API has that limitation. But with some documentation maybe that could be addressed.

    - Yeah, I haven't thought much about failing/cancelling a request from an interceptor. Will see if how I can abstract that out as well. 

Thanks for your feedback Eric once again. 

--

Regards,
-Sivabalan

[Eric Anderson]

On Mon, Jul 20, 2020 at 8:54 AM Sivabalan <n.si...@gmail.com> wrote:

On Mon, Jul 20, 2020 at 11:02 AM Eric Anderson <ej...@google.com> wrote:

This mainly supports "observing" client interceptors, that don't need to make any changes to the RPC other than Metadata. That helps many, but lots of interceptors need more power than this. It's still fine to have a simpler version in certain scenarios.

        thanks. Can. you throw some light on what more power you are insinuating.

You can have interceptors that implement retries by calling Channel next multiple times. You can have caching interceptors that respond without using Channel next by calling the Listener directly. You can have validating interceptors and fault injection interceptors that fail calls. You can have interceptors that copy data from the request into Metadata so that it can be used for load balancing. You can have interceptors that look up custom proto options from the MethodDescriptor to configure themselves. You can have interceptors change the deadline if it is too large or missing.

If it just had Metadata, especially request metadata, then its limits would be more obvious. But users frequently ask how they fail a call, and you can't fail a call with this type of API. I'm not sure users would realize the API has that limitation. But with some documentation maybe that could be addressed.

    - Yeah, I haven't thought much about failing/cancelling a request from an interceptor. Will see if how I can abstract that out as well. 

I wouldn't suggest trying to support "all the cases." The current interceptors are complex to allow "all the cases." Current interceptors see everything between the application and gRPC, so they can do whatever they want. I think the most gains would be gotten by limiting the functionality available, thus simplifying things when the use-case is supported. So you target common use cases and try to simplify those.

[Sivabalan]

yep, sounds good Eric. Yeah, if an interceptor needs complex processing, might as well implement ClientInterceptor and use any of existing supporting entities like ForwardingClientCall, SimpleForwardingClientCallListener etc. 

--

Regards,
-Sivabalan