Fix batch client batchSendLoop panic #1021

crazycs520 · 2023-10-17T02:52:35Z

panic is cause by data race:

==================
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

Signed-off-by: crazycs520 <[email protected]>

internal/client/client_batch.go

cfzjywxk · 2023-10-17T04:42:52Z

tikvrpc/tikvrpc.go

-	if !AttachContext(req, ctx) {
+
+	// Shallow copy the context to avoid concurrent modification.
+	copyCtx := req.Context


Actually the req could still be modifed in different threads?

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.

Signed-off-by: crazycs520 <[email protected]>

tikvrpc/tikvrpc.go

Signed-off-by: crazycs520 <[email protected]>

Co-authored-by: cfzjywxk <[email protected]> Co-authored-by: cfzjywxk <[email protected]> Co-authored-by: disksing <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: zzm <[email protected]> Co-authored-by: husharp <[email protected]> Co-authored-by: you06 <[email protected]> Co-authored-by: buffer <[email protected]> Co-authored-by: 3pointer <[email protected]> Co-authored-by: buffer <[email protected]> Co-authored-by: husharp <[email protected]> Co-authored-by: crazycs520 <[email protected]> Co-authored-by: Smilencer <[email protected]> Co-authored-by: ShuNing <[email protected]> Co-authored-by: zyguan <[email protected]> Co-authored-by: Jack Yu <[email protected]> Co-authored-by: Weizhen Wang <[email protected]> Co-authored-by: lucasliang <[email protected]> Co-authored-by: healthwaite <[email protected]> Co-authored-by: xufei <[email protected]> Co-authored-by: JmPotato <[email protected]> Co-authored-by: ekexium <[email protected]> Co-authored-by: 山岚 <[email protected]> Co-authored-by: glorv <[email protected]> Co-authored-by: Yongbo Jiang <[email protected]> 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)

crazycs520 added 6 commits October 16, 2023 20:21

init

41d4a7e

Signed-off-by: crazycs520 <[email protected]>

add test

ed34dd1

Signed-off-by: crazycs520 <[email protected]>

refine test

7cbd31a

Signed-off-by: crazycs520 <[email protected]>

try to fix

9d409e6

Signed-off-by: crazycs520 <[email protected]>

fix test

42dc160

Signed-off-by: crazycs520 <[email protected]>

fix

766bd18

Signed-off-by: crazycs520 <[email protected]>

crazycs520 marked this pull request as ready for review October 17, 2023 02:54

cfzjywxk requested review from cfzjywxk, zyguan, you06, MyonKeminta and ekexium October 17, 2023 04:39

refine

ad1e045

Signed-off-by: crazycs520 <[email protected]>

cfzjywxk reviewed Oct 17, 2023

View reviewed changes

address comment

6fb470c

Signed-off-by: crazycs520 <[email protected]>

zyguan reviewed Oct 17, 2023

View reviewed changes

tikvrpc/tikvrpc.go Outdated Show resolved Hide resolved

address comment

550c9dd

Signed-off-by: crazycs520 <[email protected]>

zyguan approved these changes Oct 17, 2023

View reviewed changes

cfzjywxk approved these changes Oct 17, 2023

View reviewed changes

cfzjywxk merged commit 2eaf68e into tikv:master Oct 17, 2023
10 checks passed

This was referenced Oct 17, 2023

Fix batch client batchSendLoop panic (#1021) #1022

Merged

Fix batch client batchSendLoop panic (#1021) #1023

Merged

*: update tikv/client-go to fix batch-client send loop panic issue pingcap/tidb#47716

Closed

crazycs520 added a commit to crazycs520/client-go that referenced this pull request Oct 17, 2023

Fix batch client batchSendLoop panic (tikv#1021)

2619916

Signed-off-by: crazycs520 <[email protected]>

crazycs520 mentioned this pull request Oct 17, 2023

Fix batch client batchSendLoop panic (#1021) #1024

Merged

disksing pushed a commit that referenced this pull request Oct 18, 2023

Fix batch client batchSendLoop panic (#1021) (#1024)

28b16b2

Signed-off-by: crazycs520 <[email protected]>

disksing pushed a commit that referenced this pull request Oct 18, 2023

Fix batch client batchSendLoop panic (#1021) (#1023)

65f7812

Signed-off-by: crazycs520 <[email protected]>

disksing pushed a commit that referenced this pull request Oct 18, 2023

Fix batch client batchSendLoop panic (#1021) (#1022)

916bb20

Signed-off-by: crazycs520 <[email protected]>

crazycs520 mentioned this pull request Oct 18, 2023

*: update tikv/client-go to fix batch-client send loop panic issue (#47716) pingcap/tidb#47745

Merged

5 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix batch client batchSendLoop panic #1021

Fix batch client batchSendLoop panic #1021

crazycs520 commented Oct 17, 2023 •

edited

Loading

cfzjywxk Oct 17, 2023

crazycs520 Oct 17, 2023

Fix batch client batchSendLoop panic #1021

Fix batch client batchSendLoop panic #1021

Conversation

crazycs520 commented Oct 17, 2023 • edited Loading

cfzjywxk Oct 17, 2023

Choose a reason for hiding this comment

crazycs520 Oct 17, 2023

Choose a reason for hiding this comment

crazycs520 commented Oct 17, 2023 •

edited

Loading