fix: use executor when WithExecutor is set without SetDefaultExecutor#53
fix: use executor when WithExecutor is set without SetDefaultExecutor#53
Conversation
ExecutorClientInterceptor is now always in the default client chain. When no executor is configured (neither global nor per-call), it falls back to Hystrix for backward compatibility. When WithExecutor is passed per-call or per-connection, the executor is used directly — no longer requires a global SetDefaultExecutor to activate the executor code path.
📝 WalkthroughWalkthroughThe changes refactor client interceptor wiring to ensure Changes
Sequence DiagramsequenceDiagram
participant Client
participant ExecutorClientInterceptor
participant HystrixFallback
participant Invoker
Client->>ExecutorClientInterceptor: RPC Call with options
alt Per-call executor via WithExecutor
ExecutorClientInterceptor->>Invoker: Use per-call executor
else Global executor via SetDefaultExecutor
ExecutorClientInterceptor->>Invoker: Use global executor
else Explicit opt-out (WithoutExecutor/WithoutHystrix)
ExecutorClientInterceptor->>Invoker: Direct passthrough
else No executor configured
ExecutorClientInterceptor->>HystrixFallback: Hystrix fallback
HystrixFallback->>Invoker: Execute via Hystrix circuit
Invoker-->>HystrixFallback: Result/Error
alt Hystrix error handling
HystrixFallback->>HystrixFallback: Filter errors, recover panics
end
HystrixFallback-->>ExecutorClientInterceptor: Return result
end
Invoker-->>Client: Response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 fixes client-side interceptor selection so WithExecutor activates the executor path even when no global executor has been configured, while preserving backward compatibility by falling back to Hystrix when no executor is available.
Changes:
DefaultClientInterceptorsnow always includesExecutorClientInterceptor, which internally falls back to Hystrix when no executor is configured.- Added
hystrixFallbackto preserve legacy Hystrix behavior when neither global nor per-call executors are set. - Expanded tests to cover per-call/per-connection
WithExecutorwithout a global executor and passthrough behavior forWithoutHystrix/WithoutExecutor.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| client.go | Always uses ExecutorClientInterceptor and adds Hystrix fallback behavior when no executor is configured. |
| config.go | Updates SetDefaultExecutor docs to reflect new default-chain behavior. |
| interceptors_test.go | Adds/updates tests for executor selection and fallback semantics. |
| README.md | Regenerates docs to reflect updated comments/line references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- SetDefaultExecutor doc now notes the useCBClientInterceptors gate - HystrixClientInterceptor delegates to hystrixFallback to avoid drift
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client.go (1)
174-216: Good deduplication — one observation on a now-unreachable branch.Extracting
hystrixFallbackand routing bothExecutorClientInterceptor's fallback andHystrixClientInterceptorthrough it is a clean consolidation.Minor observation: the
o.disableHystrixshort-circuit at Line 180-181 is effectively only reachable fromHystrixClientInterceptornow, becauseWithoutHystrix()also setshasExecutor=true(seeinterceptors.goLine 52-62), which makesExecutorClientInterceptorreturn the passthrough at Line 139 before ever callinghystrixFallback. That's fine as defensive coding for the direct-HystrixClientInterceptorcaller path, just worth keeping in mind ifHystrixClientInterceptoris eventually removed in v1 — the check can go with it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client.go` around lines 174 - 216, The o.disableHystrix early-return in hystrixFallback is effectively unreachable when ExecutorClientInterceptor is used (because WithoutHystrix sets hasExecutor=true), so remove the disableHystrix short-circuit from hystrixFallback and rely on callers (ExecutorClientInterceptor / HystrixClientInterceptor) to decide whether to call hystrixFallback; update/remove any tests that expect hystrixFallback to honor o.disableHystrix and ensure WithoutHystrix and hasExecutor logic remain the single source determining passthrough behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client.go`:
- Around line 174-216: The o.disableHystrix early-return in hystrixFallback is
effectively unreachable when ExecutorClientInterceptor is used (because
WithoutHystrix sets hasExecutor=true), so remove the disableHystrix
short-circuit from hystrixFallback and rely on callers
(ExecutorClientInterceptor / HystrixClientInterceptor) to decide whether to call
hystrixFallback; update/remove any tests that expect hystrixFallback to honor
o.disableHystrix and ensure WithoutHystrix and hasExecutor logic remain the
single source determining passthrough behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c81f14f4-a54e-4b94-abdd-76d7e012e4a7
📒 Files selected for processing (4)
README.mdclient.goconfig.gointerceptors_test.go
Summary
DefaultClientInterceptorsnow always usesExecutorClientInterceptorinstead of choosing between it andHystrixClientInterceptorat construction timehystrixFallbackWithExecutoris passed per-call or per-connection, the executor is used directly — no longer requiresSetDefaultExecutorto activate the executor code pathTest plan
make testwith-race)WithExecutorwithout global executorWithExecutorwithout global executorWithoutHystrixpassthrough without global executorWithoutExecutorpassthrough without global executormake lintclean (0 issues)README.mdregenerated viamake docSummary by CodeRabbit
Release Notes
Documentation
Improvements