Skip to content

Fix batch client batchSendLoop panic#1021

Merged
cfzjywxk merged 9 commits intotikv:masterfrom
crazycs520:fix-batch-conn-panic0
Oct 17, 2023
Merged

Fix batch client batchSendLoop panic#1021
cfzjywxk merged 9 commits intotikv:masterfrom
crazycs520:fix-batch-conn-panic0

Conversation

@crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Oct 17, 2023 •
edited
Loading

close #1020

panic is cause by data race:

image 
==================
WARNING: DATA RACE
Write at 0x00c0006aabfc by goroutine 1072:
  github.com/tikv/client-go/v2/internal/locate.(*RegionRequestSender).SendReqCtx()
      /Users/cs/code/goread/src/github.com/pingcap/client-go/internal/locate/region_request.go:1462 +0x1ae4
  github.com/tikv/client-go/v2/internal/locate.(*RegionRequestSender).SendReq()
      /Users/cs/code/goread/src/github.com/pingcap/client-go/internal/locate/region_request.go:243 +0x110
  github.com/tikv/client-go/v2/internal/locate.(*testRegionRequestToSingleStoreSuite).TestBatchClientSendLoopPanic.func5()
      /Users/cs/code/goread/src/github.com/pingcap/client-go/internal/locate/region_request_test.go:779 +0xbc

Previous read at 0x00c0006aabfc by goroutine 1267:
  github.com/pingcap/kvproto/pkg/kvrpcpb.(*Context).Size()
      /Users/cs/code/goread/pkg/mod/github.com/pingcap/kvproto@v0.0.0-20230904082117-ecdbf1f8c130/pkg/kvrpcpb/kvrpcpb.pb.go:21210 +0x840
  github.com/pingcap/kvproto/pkg/coprocessor.(*Request).Size()
      /Users/cs/code/goread/pkg/mod/github.com/pingcap/kvproto@v0.0.0-20230904082117-ecdbf1f8c130/pkg/coprocessor/coprocessor.pb.go:1657 +0x54
  github.com/pingcap/kvproto/pkg/tikvpb.(*BatchCommandsRequest_Request_Coprocessor).Size()
      /Users/cs/code/goread/pkg/mod/github.com/pingcap/kvproto@v0.0.0-20230904082117-ecdbf1f8c130/pkg/tikvpb/tikvpb.pb.go:6157 +0x50
  github.com/pingcap/kvproto/pkg/tikvpb.(*BatchCommandsRequest_Request).MarshalToSizedBuffer()
      /Users/cs/code/goread/pkg/mod/github.com/pingcap/kvproto@v0.0.0-20230904082117-ecdbf1f8c130/pkg/tikvpb/tikvpb.pb.go:4261 +0x110
  github.com/pingcap/kvproto/pkg/tikvpb.(*BatchCommandsRequest).MarshalToSizedBuffer()
      /Users/cs/code/goread/pkg/mod/github.com/pingcap/kvproto@v0.0.0-20230904082117-ecdbf1f8c130/pkg/tikvpb/tikvpb.pb.go:4221 +0x230
  github.com/pingcap/kvproto/pkg/tikvpb.(*BatchCommandsRequest).Marshal()
      /Users/cs/code/goread/pkg/mod/github.com/pingcap/kvproto@v0.0.0-20230904082117-ecdbf1f8c130/pkg/tikvpb/tikvpb.pb.go:4179 +0x58
  google.golang.org/protobuf/internal/impl.legacyMarshal()
      /Users/cs/code/goread/pkg/mod/google.golang.org/protobuf@v1.30.0/internal/impl/legacy_message.go:402 +0x88
  google.golang.org/protobuf/proto.MarshalOptions.marshal()
      /Users/cs/code/goread/pkg/mod/google.golang.org/protobuf@v1.30.0/proto/encode.go:166 +0x2b4
  google.golang.org/protobuf/proto.MarshalOptions.MarshalAppend()
      /Users/cs/code/goread/pkg/mod/google.golang.org/protobuf@v1.30.0/proto/encode.go:125 +0x74
  github.com/golang/protobuf/proto.marshalAppend()
      /Users/cs/code/goread/pkg/mod/github.com/golang/protobuf@v1.5.3/proto/wire.go:40 +0x90
  github.com/golang/protobuf/proto.Marshal()
      /Users/cs/code/goread/pkg/mod/github.com/golang/protobuf@v1.5.3/proto/wire.go:23 +0x58
  google.golang.org/grpc/encoding/proto.codec.Marshal()
      /Users/cs/code/goread/pkg/mod/google.golang.org/grpc@v1.54.0/encoding/proto/proto.go:45 +0x5c
  google.golang.org/grpc/encoding/proto.(*codec).Marshal()
      <autogenerated>:1 +0x44
  google.golang.org/grpc.encode()
      /Users/cs/code/goread/pkg/mod/google.golang.org/grpc@v1.54.0/rpc_util.go:632 +0x4c
  google.golang.org/grpc.prepareMsg()
      /Users/cs/code/goread/pkg/mod/google.golang.org/grpc@v1.54.0/stream.go:1753 +0x11c
  google.golang.org/grpc.(*clientStream).SendMsg()
      /Users/cs/code/goread/pkg/mod/google.golang.org/grpc@v1.54.0/stream.go:874 +0x144
  github.com/pingcap/kvproto/pkg/tikvpb.(*tikvBatchCommandsClient).Send()
      /Users/cs/code/goread/pkg/mod/github.com/pingcap/kvproto@v0.0.0-20230904082117-ecdbf1f8c130/pkg/tikvpb/tikvpb.pb.go:2130 +0x54
  github.com/tikv/client-go/v2/internal/client.(*batchCommandsClient).send()
      /Users/cs/code/goread/src/github.com/pingcap/client-go/internal/client/client_batch.go:542 +0x3f0
  github.com/tikv/client-go/v2/internal/client.(*batchConn).getClientAndSend()
      /Users/cs/code/goread/src/github.com/pingcap/client-go/internal/client/client_batch.go:396 +0x64c
  github.com/tikv/client-go/v2/internal/client.(*batchConn).batchSendLoop()
      /Users/cs/code/goread/src/github.com/pingcap/client-go/internal/client/client_batch.go:350 +0x8c
  github.com/tikv/client-go/v2/internal/client.(*connArray).Init.func10()
      /Users/cs/code/goread/src/github.com/pingcap/client-go/internal/client/client.go:323 +0x9c

@crazycs520
init
41d4a7e 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
add test
ed34dd1 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
refine test
7cbd31a 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
try to fix
9d409e6 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
fix test
42dc160 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
fix
766bd18 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520 crazycs520 marked this pull request as ready for review October 17, 2023 02:54
@crazycs520
refine
ad1e045 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
cfzjywxk
cfzjywxk reviewed Oct 17, 2023
View reviewed changes
tikvrpc/tikvrpc.go Outdated
if !AttachContext(req, ctx) {

// Shallow copy the context to avoid concurrent modification.
copyCtx := req.Context
Copy link
Contributor

@cfzjywxk cfzjywxk Oct 17, 2023

Choose a reason for hiding this comment

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

Actually the req could still be modifed in different threads?

Copy link
Contributor Author

@crazycs520 crazycs520 Oct 17, 2023

Choose a reason for hiding this comment

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

I don't find data race in req.
Currently, the data race is between req.Context and req.Req.Context, since before this PR, req.Req.Context is a reference of req.Context.

@crazycs520
address comment
6fb470c 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
zyguan
zyguan reviewed Oct 17, 2023
View reviewed changes
@crazycs520
address comment
550c9dd 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@cfzjywxk cfzjywxk merged commit 2eaf68e into tikv:master Oct 17, 2023
This was referenced Oct 17, 2023
Merged
Merged
Closed
crazycs520 added a commit to crazycs520/client-go that referenced this pull request Oct 17, 2023
@crazycs520
Fix batch client batchSendLoop panic (tikv#1021)
2619916 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520 crazycs520 mentioned this pull request Oct 17, 2023
Merged
disksing pushed a commit that referenced this pull request Oct 18, 2023
@crazycs520
Fix batch client batchSendLoop panic (#1021) (#1024)
28b16b2 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
disksing pushed a commit that referenced this pull request Oct 18, 2023
@crazycs520
Fix batch client batchSendLoop panic (#1021) (#1023)
65f7812 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
disksing pushed a commit that referenced this pull request Oct 18, 2023
@crazycs520
Fix batch client batchSendLoop panic (#1021) (#1022)
916bb20 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520 crazycs520 mentioned this pull request Oct 18, 2023
Merged
5 tasks
iosmanthus added a commit that referenced this pull request Dec 20, 2023
@iosmanthus @cfzjywxk
@disksing @dependabot @zeminzhou @HuSharp @you06 @bufferflies @3pointer @crazycs520 @Smityz @nolouch @zyguan @jackysp @hawkingrei @LykxSassinator @healthwaite @windtalker @JmPotato @ekexium @YuJuncen @glorv @CabinfeverB
merge master for cse region client (#1083)
0041484 
Co-authored-by: cfzjywxk <cfzjywxk@gmail.com>
Co-authored-by: cfzjywxk <lsswxrxr@163.com>
Co-authored-by: disksing <i@disksing.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: zzm <zhouzemin@pingcap.com>
Co-authored-by: husharp <jinhao.hu@pingcap.com>
Co-authored-by: you06 <you1474600@gmail.com>
Co-authored-by: buffer <doufuxiaowangzi@gmail.com>
Co-authored-by: 3pointer <qdlc2010@gmail.com>
Co-authored-by: buffer <1045931706@qq.com>
Co-authored-by: husharp <ihusharp@gmail.com>
Co-authored-by: crazycs520 <crazycs520@gmail.com>
Co-authored-by: Smilencer <smityz@qq.com>
Co-authored-by: ShuNing <nolouch@gmail.com>
Co-authored-by: zyguan <zhongyangguan@gmail.com>
Co-authored-by: Jack Yu <jackysp@gmail.com>
Co-authored-by: Weizhen Wang <wangweizhen@pingcap.com>
Co-authored-by: lucasliang <nkcs_lykx@hotmail.com>
Co-authored-by: healthwaite <148101100+healthwaite@users.noreply.github.com>
Co-authored-by: xufei <xufeixw@mail.ustc.edu.cn>
Co-authored-by: JmPotato <ghzpotato@gmail.com>
Co-authored-by: ekexium <eke@fastmail.com>
Co-authored-by: 山岚 <36239017+YuJuncen@users.noreply.github.com>
Co-authored-by: glorv <glorvs@163.com>
Co-authored-by: Yongbo Jiang <cabinfeveroier@gmail.com>
resolve locks interface for tidb gc_worker (#945)
fix some issues of replica selector (#910)  (#942)
fix some issues of replica selector (#910)
fix issue of configure kv timeout not work when disable batch client (#980)
fix batch-client wait too long and add some metrics (#973)
fix batch-client wait too long and add some metrics (#973)" (#984)
fix data race at the aggressiveLockingDirty (#913)
fix MinSafeTS might be set to MaxUint64 permanently (#994)
fix: fix invalid nil pointer when trying to record Store.SlownessStat. (#1017)
Fix batch client batchSendLoop panic (#1021)
fix request source tag unset (#1025)
Fix comment of `SuspendTime` (#1057)
@ti-chi-bot ti-chi-bot added needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. and removed needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. labels May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cfzjywxk cfzjywxk cfzjywxk approved these changes

@you06 you06 Awaiting requested review from you06

@MyonKeminta MyonKeminta Awaiting requested review from MyonKeminta

@ekexium ekexium Awaiting requested review from ekexium

+1 more reviewer

@zyguan zyguan zyguan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

batch client batchSendLoop panic

4 participants

@crazycs520 @cfzjywxk @zyguan @ti-chi-bot