Simplify chain interceptors #421
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
The four (client/server, unary/stream) interceptors have to wrap a slice of interceptors in functions which satisfy the handler interface, but which are closures over the other parameters an interceptor is expected to have. The previous approach accomplished this goal with recursion. This had two drawbacks: first, the code was difficult to understand, as most recursion attempts to fully encode state in function parameters, but here state was necessarily also encoded in the closure; and second, the recursive base-case meant that even the innermost interceptor was not calling the bare handler, it was calling a wrapped handler. This new approach instead iteratively constructs wrappers from the inside out. It results in fewer lines of code, with fewer variables held in each closure. Hopefully this results in higher readability.
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
Codecov Report
@@ Coverage Diff @@
## master #421 +/- ##
==========================================
- Coverage 74.37% 73.21% -1.17%
==========================================
Files 42 42
Lines 1354 1370 +16
==========================================
- Hits 1007 1003 -4
- Misses 294 306 +12
- Partials 53 61 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hi Aaron, thanks for your contribution. I want to note that this logic is going to be abandoned soon for two reasons:
- We're working on v2 of the middleware, which does not use this code because...
- There is an official implementation of this in the grpc-go library. See https://pkg.go.dev/google.golang.org/grpc#WithChainStreamInterceptor etc.
With that in mind, are you still interested in contributing this? The change looks good to me, and thanks for adding all the colorful comments :).
|
Apologies for the delay! Yes, I'm still happy to contribute this; as far as I'm concerned its much more up to you as to whether you want churn in this code shortly before it is removed :) Happy for this to be merged, or for it to be closed, at your discretion! |
|
I'm happy enough to merge it, thanks for your contribution! |
The four (client/server, unary/stream) interceptors have to wrap a slice
of interceptors in functions which satisfy the handler interface, but
which are closures over the other parameters an interceptor is expected
to have.
The previous approach accomplished this goal with recursion. This had
two drawbacks: first, the code was difficult to understand, as most
recursion attempts to fully encode state in function parameters, but
here state was necessarily also encoded in the closure; and second,
the recursive base-case meant that even the innermost interceptor was
not calling the bare handler, it was calling a wrapped handler.
This new approach instead iteratively constructs wrappers from the
inside out. It results in fewer lines of code, with fewer variables
held in each closure. Hopefully this leads to higher readability.
It also makes it so that the innermost interceptor is calling the gRPC
handler directly, allowing introspection using
reflect.ValueOf(handler)to return the actual underlying handler instead of an anonymous func.