perf: cache interceptor chain in DoHTTPtoGRPC#22
Conversation
DoHTTPtoGRPC was rebuilding the entire interceptor chain on every call via ChainUnaryServer(DefaultInterceptors()...). Since the chain is deterministic after init, cache it with sync.Once to avoid per-request slice allocations and closure creation.
📝 WalkthroughWalkthroughIntroduces a package-level, lazily initialized cached gRPC unary interceptor (using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the DoHTTPtoGRPC path by avoiding per-call reconstruction of the gRPC unary interceptor chain, under the assumption that interceptor configuration is fixed after initialization.
Changes:
- Add a
sync.Once-backed cache for the chained unary server interceptor used byDoHTTPtoGRPC - Introduce
getHTTPtoGRPCInterceptor()to lazily initialize and reuse the chain - Update
DoHTTPtoGRPCto use the cached interceptor chain
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Document that interceptor configuration must be finalized before the first DoHTTPtoGRPC call, consistent with the init-only contract on all other configuration functions in this package.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
interceptors.go (1)
135-145: Consider fail-fast enforcement after cache freeze.Once the chain is initialized, later calls to config mutators (
AddUnaryServerInterceptor,SetFilterFunc, etc.) are silently ignored for this path. A guard would make misconfiguration obvious at runtime.Proposed refactor (fail-fast freeze guard)
import ( "context" stdError "errors" "fmt" "net/http" "runtime/debug" "strings" "sync" + "sync/atomic" "time" @@ var ( httpToGRPCOnce sync.Once httpToGRPCInterceptor grpc.UnaryServerInterceptor + httpToGRPCFrozen atomic.Bool ) func getHTTPtoGRPCInterceptor() grpc.UnaryServerInterceptor { httpToGRPCOnce.Do(func() { + httpToGRPCFrozen.Store(true) httpToGRPCInterceptor = grpc_middleware.ChainUnaryServer(DefaultInterceptors()...) }) return httpToGRPCInterceptor } + +func assertHTTPtoGRPCMutable(caller string) { + if httpToGRPCFrozen.Load() { + panic(caller + " called after first DoHTTPtoGRPC invocation") + } +}Then call
assertHTTPtoGRPCMutable(...)at the top of mutators (SetFilterFunc,AddUnaryServerInterceptor, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interceptors.go` around lines 135 - 145, The HTTP->gRPC interceptor chain is lazily initialized via httpToGRPCOnce in getHTTPtoGRPCInterceptor, so subsequent mutators like AddUnaryServerInterceptor and SetFilterFunc silently no-op; add a fail-fast guard by introducing a freeze flag (e.g., an atomic.Bool httpToGRPCFrozen) and a helper assertHTTPtoGRPCMutable() that checks the flag and panics/logs a clear error if frozen, set httpToGRPCFrozen = true inside the httpToGRPCOnce.Do closure (in getHTTPtoGRPCInterceptor) after initializing httpToGRPCInterceptor, and call assertHTTPtoGRPCMutable() at the start of mutators (AddUnaryServerInterceptor, SetFilterFunc, etc.) to make misconfiguration obvious at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@interceptors.go`:
- Around line 135-145: The HTTP->gRPC interceptor chain is lazily initialized
via httpToGRPCOnce in getHTTPtoGRPCInterceptor, so subsequent mutators like
AddUnaryServerInterceptor and SetFilterFunc silently no-op; add a fail-fast
guard by introducing a freeze flag (e.g., an atomic.Bool httpToGRPCFrozen) and a
helper assertHTTPtoGRPCMutable() that checks the flag and panics/logs a clear
error if frozen, set httpToGRPCFrozen = true inside the httpToGRPCOnce.Do
closure (in getHTTPtoGRPCInterceptor) after initializing httpToGRPCInterceptor,
and call assertHTTPtoGRPCMutable() at the start of mutators
(AddUnaryServerInterceptor, SetFilterFunc, etc.) to make misconfiguration
obvious at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92e8ef23-a56f-47c4-ac09-f5de60faac1d
📒 Files selected for processing (2)
README.mdinterceptors.go
✅ Files skipped from review due to trivial changes (1)
- README.md
Summary
DoHTTPtoGRPCviasync.Onceinstead of rebuilding it on every callTest plan
make buildpassesmake testpasses (go test -race ./...)make lintpasses (golangci-lint 0 issues + govulncheck 0 vulnerabilities)Summary by CodeRabbit