optimize: disable rpcinfo pool by default - #1973
Open
ChocoLZS wants to merge 9 commits into
Open
Conversation
ChocoLZS
marked this pull request as draft
June 29, 2026 02:04
ChocoLZS
changed the title
ChocoLZS
force-pushed
the
feat/rpcinfo-pool-disabled-by-default
branch
3 times, most recently
from
June 29, 2026 05:10
150aa03 to
f3c2150
Compare
ChocoLZS
marked this pull request as ready for review
June 30, 2026 10:30
ChocoLZS
force-pushed
the
feat/rpcinfo-pool-disabled-by-default
branch
from
August 6, 2026 04:51
1dbfa28 to
8bbc0d8
Compare
There was a problem hiding this comment.
Pull request overview
This PR changes Kitex’s RPCInfo pooling behavior to be disabled by default to avoid panics/data races when RPCInfo is accessed asynchronously after framework cleanup (per Issue #1958), while preserving opt-in compatibility via environment switches and deprecated APIs.
Changes:
- Disable RPCInfo pooling by default; add
KITEX_ENABLE_RPCINFO_POOL(and keepKITEX_DISABLE_RPCINFO_POOL) to control the legacy pooling during migration. - Guard server-side tracer finish/reset logic so RPCStats isn’t reset when pooling is disabled (avoids racing with async readers).
- Expand unit tests across client/server/transports to assert RPCInfo remains readable when pooling is disabled, and adjust existing tests to account for the new default.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service_inline_test.go | Adds async RPCInfo-read test around service-inline server finish lifecycle. |
| server/server_test.go | Ensures tests that require pooling explicitly enable/restore it. |
| server/local_caller_test.go | Adds async RPCInfo-read test for LocalCaller finish lifecycle. |
| pkg/rpcinfo/rpcstats.go | Adds deprecation/migration notes to legacy stats/event pooling. |
| pkg/rpcinfo/rpcinfo.go | Disables pooling by default; adds env-based initialization and deprecation notes. |
| pkg/rpcinfo/rpcinfo_test.go | Adds tests for env initialization behavior and precedence. |
| pkg/rpcinfo/rpcinfo_inline.go | Adds deprecation notes for inline RPCInfo pooling/recycling APIs. |
| pkg/rpcinfo/rpcconfig.go | Adds deprecation/migration notes to rpcConfig pooling/recycling. |
| pkg/rpcinfo/remoteinfo/remoteInfo.go | Adds deprecation/migration notes to remoteInfo pooling/recycling. |
| pkg/rpcinfo/invocation.go | Adds deprecation/migration notes to invocation pooling/recycling. |
| pkg/rpcinfo/endpointInfo.go | Adds deprecation/migration notes to endpointInfo pooling/recycling. |
| pkg/rpcinfo/ctx.go | Adds deprecation notes to PutRPCInfo (legacy recycling). |
| pkg/rpcinfo/ctx_test.go | Refactors RPCInfo creation helper and adds pooling on/off assertions. |
| pkg/remote/trans/nphttp2/server_handler_test.go | Adds coverage ensuring RPCInfo remains readable after unary finish when pooling is disabled. |
| pkg/remote/trans/netpollmux/server_handler.go | Avoids resetting RPCStats on finish when pooling is disabled. |
| pkg/remote/trans/netpollmux/server_handler_test.go | Adds mux-server test ensuring RPCInfo readability after finish with pooling disabled. |
| pkg/remote/trans/default_server_handler.go | Avoids resetting RPCStats on finish when pooling is disabled; documents rationale. |
| pkg/remote/trans/default_server_handler_test.go | Updates InitOrReset mock and adds test for readability after OnRead with pooling disabled. |
| client/service_inline_test.go | Adds client-side service-inline test asserting RPCInfo remains readable after call. |
| client/rpctimeout_test.go | Adds helpers and a timeout-path test asserting RPCInfo remains readable post-timeout with pooling disabled. |
| client/client_test.go | Adds multi-scenario lifecycle test ensuring async RPCInfo reads don’t panic across retry paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func newCtxTestRPCInfo() rpcinfo.RPCInfo { | ||
| method := "TestMethod" | ||
| svcName := "TestServiceName" | ||
| netAddr := utils.NewNetAddr("TestNetWork", "TestAddress") |
Comment on lines
+197
to
+199
| func TestCallDisablePoolKeepsRPCInfoReadableAcrossLifecycle(t *testing.T) { | ||
| mockErr := errors.New("mock") | ||
| testcases := []struct { |
Comment on lines
+198
to
+201
| func TestServiceInlineDisablePoolNoRaceWithAsyncServerRPCInfoReadDuringFinish(t *testing.T) { | ||
| stop := make(chan struct{}) | ||
| started := make(chan struct{}) | ||
| done := make(chan any, 1) |
Comment on lines
+310
to
+313
| func TestLocalCallerDisablePoolNoRaceWithAsyncRPCInfoReadDuringFinish(t *testing.T) { | ||
| stop := make(chan struct{}) | ||
| started := make(chan struct{}) | ||
| done := make(chan any, 1) |
Comment on lines
+214
to
+216
| func TestDefaultSvrTransHandlerDisablePoolKeepsRPCInfoReadableAfterOnRead(t *testing.T) { | ||
| buf := remote.NewReaderWriterBuffer(1024) | ||
| ext := &MockExtension{ |
Comment on lines
+558
to
+559
| func TestDisablePoolKeepsRPCInfoReadableAfterUnaryFinish(t *testing.T) { | ||
| opt := newMockServerOption() |
Comment on lines
+297
to
+299
| func TestMuxSvrOnReadDisablePoolKeepsRPCInfoReadableAfterFinish(t *testing.T) { | ||
| const body = "hello world" | ||
| buf := netpoll.NewLinkBuffer(1024) |
Comment on lines
+87
to
+90
| func TestServiceInlineDisablePoolKeepsClientRPCInfoReadableAfterCall(t *testing.T) { | ||
| ctrl := gomock.NewController(t) | ||
| defer ctrl.Finish() | ||
|
|
Comment on lines
+191
to
+196
| func TestRpcTimeoutMWDisablePoolKeepsRPCInfoReadableAfterTimeout(t *testing.T) { | ||
| timeoutCtxCh := make(chan context.Context, 1) | ||
| stop := make(chan struct{}) | ||
| started := make(chan struct{}) | ||
| done := make(chan any, 1) | ||
| mw := rpcTimeoutMW(context.Background()) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (8)
server/local_caller_test.go:310
- Typo in test name: "Aceess" should be "Access" for readability and to avoid confusing
go testoutput.
func TestRPCInfoAceessNoRace(t *testing.T) {
client/service_inline_test.go:87
- Typo in test name: "Aceess" should be "Access" for readability and to avoid confusing
go testoutput.
func TestServiceInlineRPCInfoAceessNoRace(t *testing.T) {
client/rpctimeout_test.go:191
- Typo in test name: "Aceess" should be "Access" for readability and to avoid confusing
go testoutput.
func TestRpcTimeoutMWRPCInfoAceessNoRace(t *testing.T) {
client/client_test.go:197
- Typo in test name: "Aceess" should be "Access" for readability and to avoid confusing
go testoutput.
func TestClientRPCInfoAceessNoRace(t *testing.T) {
pkg/remote/trans/netpollmux/server_handler_test.go:297
- Typo in test name: "Aceess" should be "Access" for readability and to avoid confusing
go testoutput.
func TestRPCInfoAceessNoRace(t *testing.T) {
server/service_inline_test.go:198
- Typo in test name: "Aceess" should be "Access" for readability and to avoid confusing
go testoutput.
func TestRPCInfoAceessNoRace(t *testing.T) {
pkg/remote/trans/default_server_handler_test.go:214
- Typo in test name: "Aceess" should be "Access" for readability and to avoid confusing
go testoutput.
func TestRPCInfoAceessNoRace(t *testing.T) {
pkg/remote/trans/nphttp2/server_handler_test.go:558
- Typo in test name: "Aceess" should be "Access" for readability and to avoid confusing
go testoutput.
func TestRPCInfoAceessNoRace(t *testing.T) {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (10)
server/service_inline_test.go:143
- Typo in test name: "Aceess" should be "Access" for readability and consistency (e.g., grep/test filtering).
func TestRPCInfoAceessNoRace(t *testing.T) {
server/local_caller_test.go:274
readeris assigned inside middleware; if the middleware isn't executed,readerstays nil andreader.StopAndAssertwill panic with a nil-pointer dereference. Add an explicit assertion before stopping.
reader.StopAndAssert(t)
server/service_inline_test.go:169
readeris assigned inside middleware; if the middleware chain changes or isn't executed,readerstays nil andreader.StopAndAssertwill panic with a nil-pointer dereference. Add an explicit assertion before stopping.
reader.StopAndAssert(t)
server/local_caller_test.go:255
- Typo in test name: "Aceess" should be "Access" for readability and consistency (e.g., grep/test filtering).
func TestRPCInfoAceessNoRace(t *testing.T) {
pkg/remote/trans/nphttp2/server_handler_test.go:507
- Typo in test name: "Aceess" should be "Access" for readability and consistency (e.g., grep/test filtering).
func TestRPCInfoAceessNoRace(t *testing.T) {
pkg/remote/trans/netpollmux/server_handler_test.go:246
- Typo in test name: "Aceess" should be "Access" for readability and consistency (e.g., grep/test filtering).
func TestRPCInfoAceessNoRace(t *testing.T) {
pkg/remote/trans/default_server_handler_test.go:163
- Typo in test name: "Aceess" should be "Access" for readability and consistency (e.g., grep/test filtering).
func TestRPCInfoAceessNoRace(t *testing.T) {
client/client_test.go:186
- Typo in test name: "Aceess" should be "Access" for readability and consistency (e.g., grep/test filtering).
func TestClientRPCInfoAceessNoRace(t *testing.T) {
client/service_inline_test.go:88
- Typo in test name: "Aceess" should be "Access" for readability and consistency (e.g., grep/test filtering).
func TestServiceInlineRPCInfoAceessNoRace(t *testing.T) {
client/rpctimeout_test.go:118
- Typo in test name: "Aceess" should be "Access" for readability and consistency (e.g., grep/test filtering).
func TestRpcTimeoutMWRPCInfoAceessNoRace(t *testing.T) {
junliurs
reviewed
Aug 6, 2026
junliurs
previously approved these changes
Aug 7, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1973 +/- ##
==========================================
- Coverage 62.95% 62.95% -0.01%
==========================================
Files 394 397 +3
Lines 30267 30416 +149
==========================================
+ Hits 19056 19148 +92
- Misses 9924 9984 +60
+ Partials 1287 1284 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
optimize
Check the PR title.
(Optional) Translate the PR title into Chinese.
(Optional) More detailed description for this PR(en: English/zh: Chinese).
en:
zh(optional):
(Optional) Which issue(s) this PR fixes:
Related to #1958
(optional) The PR that updates user documentation:
cloudwego/cloudwego.github.io#1559