Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

interceptor: new APIs for chaining client interceptors. #2696

Merged
merged 10 commits into from
Apr 15, 2019

Conversation

WeiranFang
Copy link
Member

@WeiranFang WeiranFang commented Mar 18, 2019

Add two new APIs in dialoptions:

  • WithChainUnaryInterceptor // Chainable client unary interceptors
  • WithChainStreamInterceptor // Chainable client stream interceptors

The chaining interceptors will be appended after the original interceptor defined by WithUnaryInterceptor and WithStreamInterceptor. The interceptors will be executed in a left-to-right order. e.g. For a chaining interceptors [int1, int2, int3], we will execute int1, and then int2, int3, before sending out the requests.

This provides the similar functionality as https://godoc.org/github.com/grpc-ecosystem/go-grpc-middleware#hdr-Chaining

API names are TBD. Please let me know if there are better names for these two APIs. Thanks!

Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interceptors will be executed in a left-to-right order

"left-to-right" is not clear to me. Instead, we should define which is the inner-most and which is outer-most.

In your tests, it seems left is assumed to be outer-most, so it both first and also last (it wraps other interceptors).

call_test.go Show resolved Hide resolved
call.go Outdated Show resolved Hide resolved
@WeiranFang
Copy link
Member Author

"left-to-right" is not clear to me. Instead, we should define which is the inner-most and which is outer-most.
In your tests, it seems left is assumed to be outer-most, so it both first and also last (it wraps other interceptors).

Yes the left-most interceptor passed in is the outer-most, and the right-most is the inner-most. I have updated the doc string to avoid confusion.

Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, some minor comments.

dialoptions.go Show resolved Hide resolved
call_test.go Outdated Show resolved Hide resolved
call_test.go Show resolved Hide resolved
call.go Outdated
@@ -39,19 +39,20 @@ func (cc *ClientConn) Invoke(ctx context.Context, method string, args, reply int
interceptors = append([]UnaryClientInterceptor{cc.dopts.unaryInt}, interceptors...)
}

return getChainInvoker(interceptors, invoke)(ctx, method, args, reply, cc, opts...)
if len(interceptors) > 0 {
return interceptors[0](ctx, method, args, reply, cc, getChainInvoker(interceptors, 0, invoke), opts...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still re-chains the interceptors for each RPC.
Chain can done in Dial (either while specifying dial options, or after processing all dial options). Then there will be only one interceptor, and can be used by all RPCs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... I misunderstood it before. I have moved the chaining logic to clientconn init time.

@dfawley dfawley self-assigned this Mar 28, 2019
@WeiranFang
Copy link
Member Author

Just a follow up, is there anything else I need to do for this PR?

@menghanl menghanl merged commit 776edd3 into grpc:master Apr 15, 2019
@menghanl menghanl changed the title New APIs for chaining client interceptors. interceptor: new APIs for chaining client interceptors. Apr 15, 2019
@menghanl menghanl added the Type: Feature New features or improvements in behavior label Apr 15, 2019
@menghanl menghanl added this to the 1.21 Release milestone Apr 15, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants