Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

fix data race in import unit(loader) #349

Merged
merged 9 commits into from
Nov 14, 2019
Merged

fix data race in import unit(loader) #349

merged 9 commits into from
Nov 14, 2019

Conversation

IANTHEREAL
Copy link
Collaborator

@IANTHEREAL IANTHEREAL commented Nov 11, 2019 •
edited
Loading

What problem does this PR solve?

find a data race in import unit(loader)

panic: send on closed channel

goroutine 44191 [running]:
github.com/pingcap/dm/loader.(*Worker).run.func1()
	/home/jenkins/agent/workspace/build_dm_master/go/src/github.com/pingcap/dm/loader/loader.go:155 +0x47b
created by github.com/pingcap/dm/loader.(*Worker).run
	/home/jenkins/agent/workspace/build_dm_master/go/src/github.com/pingcap/dm/loader/loader.go:175 +0x2ba
panic: send on closed channel

What is changed and how it works?

  • make sure the worker clean up all its goroutine when it exits
  • It's very weird, dump unit start a doJob groutine for each dump files, not for each worker

@IANTHEREAL IANTHEREAL added the status/WIP This PR is still work in progress label Nov 11, 2019
@IANTHEREAL
Copy link
Collaborator Author

/run-all-tests

@codecov
Copy link

codecov bot commented Nov 11, 2019 •
edited
Loading

Codecov Report

Merging #349 into master will increase coverage by 0.7938%.
The diff coverage is 8.1967%.

@@               Coverage Diff                @@
##             master       #349        +/-   ##
================================================
+ Coverage   56.7259%   57.5197%   +0.7938%     
================================================
  Files           159        159                
  Lines         16310      16071       -239     
================================================
- Hits           9252       9244         -8     
+ Misses         6151       5924       -227     
+ Partials        907        903         -4

@IANTHEREAL IANTHEREAL added status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix and removed status/WIP This PR is still work in progress labels Nov 11, 2019
csuzhangxc
csuzhangxc reviewed Nov 11, 2019
View reviewed changes
loader/loader.go Show resolved Hide resolved
loader/loader.go Outdated Show resolved Hide resolved
@csuzhangxc
Copy link
Member

@WangXiangUSTC PTAL

@csuzhangxc csuzhangxc mentioned this pull request Nov 13, 2019
Merged
@csuzhangxc csuzhangxc added priority/normal Minor change, requires approval from ≥1 primary reviewer needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Nov 13, 2019
@WangXiangUSTC
Copy link
Contributor

rest LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Nov 13, 2019
@WangXiangUSTC
Copy link
Contributor

LGTM

csuzhangxc
csuzhangxc reviewed Nov 13, 2019
View reviewed changes
loader/loader.go Show resolved Hide resolved
loader/loader.go Show resolved Hide resolved
csuzhangxc
csuzhangxc reviewed Nov 13, 2019
View reviewed changes
loader/loader.go Outdated Show resolved Hide resolved
loader/loader.go Show resolved Hide resolved
tests/import_goroutine_leak/run.sh Show resolved Hide resolved
@IANTHEREAL
Copy link
Collaborator Author

/run-all-tests

@csuzhangxc
worker: fix data race
3b5b1aa 
```
2019-11-13T14:04:06.914Z] WARNING: DATA RACE

[2019-11-13T14:04:06.914Z] Write at 0x00c0003bb5a0 by goroutine 97:

[2019-11-13T14:04:06.914Z]   github.com/pingcap/dm/dm/worker.(*SubTask).Init()

[2019-11-13T14:04:06.914Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/subtask.go:111 +0xf5

[2019-11-13T14:04:06.914Z]   github.com/pingcap/dm/dm/worker.(*SubTask).Run()

[2019-11-13T14:04:06.914Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/subtask.go:169 +0x2cc

[2019-11-13T14:04:06.914Z]   github.com/pingcap/dm/dm/worker.(*SubTask).Resume()

[2019-11-13T14:04:06.914Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/subtask.go:412 +0x80e

[2019-11-13T14:04:06.914Z]   github.com/pingcap/dm/dm/worker.(*Worker).handleTask()

[2019-11-13T14:04:06.914Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/worker.go:880 +0x1ecc

[2019-11-13T14:04:06.914Z]   github.com/pingcap/dm/dm/worker.(*Worker).Start.func1()

[2019-11-13T14:04:06.914Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/worker.go:178 +0x80

[2019-11-13T14:04:06.914Z]

[2019-11-13T14:04:06.914Z] Previous read at 0x00c0003bb5a0 by goroutine 147:

[2019-11-13T14:04:06.914Z]   github.com/pingcap/dm/dm/worker.(*Worker).doFetchDDLInfo()

[2019-11-13T14:04:06.914Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/worker.go:398 +0x203

[2019-11-13T14:04:06.914Z]   github.com/pingcap/dm/dm/worker.(*Worker).FetchDDLInfo.func1()

[2019-11-13T14:04:06.914Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/worker.go:374 +0x98

[2019-11-13T14:04:06.915Z]

[2019-11-13T14:04:06.915Z] Goroutine 97 (running) created at:

[2019-11-13T14:04:06.915Z]   github.com/pingcap/dm/dm/worker.(*Worker).Start()

[2019-11-13T14:04:06.915Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/worker.go:176 +0x26f

[2019-11-13T14:04:06.915Z]   github.com/pingcap/dm/dm/worker.(*Server).Start.func1()

[2019-11-13T14:04:06.915Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/server.go:87 +0x96

[2019-11-13T14:04:06.915Z]

[2019-11-13T14:04:06.915Z] Goroutine 147 (running) created at:

[2019-11-13T14:04:06.915Z]   github.com/pingcap/dm/dm/worker.(*Worker).FetchDDLInfo()

[2019-11-13T14:04:06.915Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/worker.go:372 +0x1f4

[2019-11-13T14:04:06.915Z]   github.com/pingcap/dm/dm/worker.(*Server).FetchDDLInfo()

[2019-11-13T14:04:06.915Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/worker/server.go:286 +0x28b

[2019-11-13T14:04:06.915Z]   github.com/pingcap/dm/dm/pb._Worker_FetchDDLInfo_Handler()

[2019-11-13T14:04:06.915Z]       /home/jenkins/agent/workspace/dm_ghpr_test/go/src/github.com/pingcap/dm/dm/pb/dmworker.pb.go:3865 +0xe5

[2019-11-13T14:04:06.915Z]   google.golang.org/grpc.(*Server).processStreamingRPC()

[2019-11-13T14:04:06.915Z]       /go/pkg/mod/google.golang.org/[email protected]/server.go:1199 +0x15f2

[2019-11-13T14:04:06.915Z]   google.golang.org/grpc.(*Server).handleStream()

[2019-11-13T14:04:06.915Z]       /go/pkg/mod/google.golang.org/[email protected]/server.go:1279 +0x12e5

[2019-11-13T14:04:06.915Z]   google.golang.org/grpc.(*Server).serveStreams.func1.1()

[2019-11-13T14:04:06.915Z]       /go/pkg/mod/google.golang.org/[email protected]/server.go:710 +0xac
```
@csuzhangxc
Copy link
Member

/run-all-tests tidb=release-3.0

csuzhangxc
csuzhangxc approved these changes Nov 14, 2019
View reviewed changes
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Nov 14, 2019
@IANTHEREAL IANTHEREAL merged commit fcaab27 into pingcap:master Nov 14, 2019
@IANTHEREAL IANTHEREAL deleted the import-data-race branch November 14, 2019 04:49
@sre-bot sre-bot mentioned this pull request Nov 14, 2019
Merged
@sre-bot
Copy link

sre-bot commented Nov 14, 2019

cherry pick to release-1.0 in PR #353

@sre-bot sre-bot added already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 labels Nov 14, 2019
IANTHEREAL pushed a commit that referenced this pull request Nov 15, 2019
@sre-bot @IANTHEREAL
fix data race in import unit(loader) (#349) (#353)
fb6cd98
@ericsyh ericsyh added this to the v1.0.2 milestone Nov 22, 2019
@ericsyh ericsyh removed this from the v1.0.3 milestone Dec 3, 2019
@csuzhangxc csuzhangxc added already-update-release-note The release note is updated. Add this label once the release note is updated and removed needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Mar 13, 2020
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
@IANTHEREAL
fix data race in import unit(loader) (pingcap#349)
a3fbd51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@WangXiangUSTC WangXiangUSTC WangXiangUSTC left review comments

@csuzhangxc csuzhangxc csuzhangxc approved these changes

Assignees
No one assigned
Labels
already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked already-update-release-note The release note is updated. Add this label once the release note is updated priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@IANTHEREAL @csuzhangxc @WangXiangUSTC @sre-bot @ericsyh