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

worker: fix potential panic when restoring subtask from meta (#305) #311

Merged
merged 3 commits into from
Oct 16, 2019

Conversation

sre-bot
Copy link

@sre-bot sre-bot commented Oct 16, 2019

cherry-pick #305 to release-1.0


What problem does this PR solve?

[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x53b07e]

goroutine 47 [running]:
context.WithDeadline(0x0, 0x0, 0xbf5be448e73dc52d, 0x463dd73766, 0x2ad5a60, 0x0, 0x0, 0x0)
/usr/local/go/src/context/context.go:384 +0x4e
context.WithTimeout(0x0, 0x0, 0x45d964b800, 0xc001974200, 0x2, 0x2)
/usr/local/go/src/context/context.go:451 +0x6b
github.com/pingcap/dm/dm/worker.(*SubTask).unitTransWaitCondition(0xc000372e40, 0x0, 0x0)
/home/jenkins/workspace/build_dm_master/go/src/github.com/pingcap/dm/dm/worker/subtask.go:627 +0x3ca
github.com/pingcap/dm/dm/worker.(*SubTask).run(0xc000372e40)
/home/jenkins/workspace/build_dm_master/go/src/github.com/pingcap/dm/dm/worker/subtask.go:182 +0x5c
github.com/pingcap/dm/dm/worker.(*SubTask).fetchResult(0xc000372e40, 0xc0004a7560)
/home/jenkins/workspace/build_dm_master/go/src/github.com/pingcap/dm/dm/worker/subtask.go:248 +0xb28
created by github.com/pingcap/dm/dm/worker.(*SubTask).run
/home/jenkins/workspace/build_dm_master/go/src/github.com/pingcap/dm/dm/worker/subtask.go:196 +0x4a8

What is changed and how it works?

before this PR, we have the following order in worker.go:

  1. InitConditionHub registers Worker instances
  2. restoreSubTask restores subtasks from meta
  3. w.ctx, w.cancel = context.WithCancel(context.Background()) inits w.ctx and w.cancel

but when restoring subtasks from meta, w.ctx may be used, then nil pointer will be accessed and panicked.

in this PR, we make sure have the following order:

  1. w.ctx, w.cancel = context.WithCancel(context.Background()) inits w.ctx and w.cancel
  2. InitConditionHub registers Worker instances
  3. restoreSubTask restores subtasks from meta

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

@sre-bot sre-bot added priority/normal Minor change, requires approval from ≥1 primary reviewer needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated type/bug-fix Bug fix type/cherry-pick This PR is just a cherry-pick (backport) labels Oct 16, 2019
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

LGTM

@amyangfei amyangfei added the status/LGT1 One reviewer already commented LGTM label Oct 16, 2019
@codecov
Copy link

codecov bot commented Oct 16, 2019

Codecov Report

Merging #311 into release-1.0 will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           release-1.0       #311   +/-   ##
==============================================
  Coverage      60.1916%   60.1916%           
==============================================
  Files              134        134           
  Lines            14816      14816           
==============================================
  Hits              8918       8918           
  Misses            5059       5059           
  Partials           839        839

@csuzhangxc csuzhangxc added the priority/release-blocker This PR blocks a release. Please review it ASAP. label Oct 16, 2019
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Oct 16, 2019
@csuzhangxc csuzhangxc merged commit db4cb60 into pingcap:release-1.0 Oct 16, 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 Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 priority/release-blocker This PR blocks a release. Please review it ASAP. status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix type/cherry-pick This PR is just a cherry-pick (backport)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants