New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: fix a data race. #706

Merged
merged 3 commits into from Aug 22, 2017

Conversation

Projects
None yet
4 participants
@disksing
Member

disksing commented Aug 16, 2017

Operators may be accessed concurrently, should be mutex guarded.

@disksing disksing requested review from siddontang and huachaohuang Aug 16, 2017

Show outdated Hide outdated server/operator.go Outdated
@huachaohuang

This comment has been minimized.

Show comment
Hide comment
@huachaohuang

huachaohuang Aug 16, 2017

Member

What cause the data race?
Seems not a good idea to add lock to every operator?

Member

huachaohuang commented Aug 16, 2017

What cause the data race?
Seems not a good idea to add lock to every operator?

@disksing

This comment has been minimized.

Show comment
Hide comment
@disksing

disksing Aug 16, 2017

Member

@huachaohuang
Here for instance:

pd/server/coordinator.go

Lines 96 to 106 in 9d2d2ae

if op := c.getOperator(region.GetId()); op != nil {
res, finished := op.Do(region)
if !finished {
collectOperatorCounterMetrics(op)
if res != nil {
c.hbStreams.sendMsg(region, res)
}
return
}
c.removeOperator(op)
}

Although getOperator is protected inside, we need to update Operator after, without any lock. If another goroutine wants to read it, then there will be a data race.

Member

disksing commented Aug 16, 2017

@huachaohuang
Here for instance:

pd/server/coordinator.go

Lines 96 to 106 in 9d2d2ae

if op := c.getOperator(region.GetId()); op != nil {
res, finished := op.Do(region)
if !finished {
collectOperatorCounterMetrics(op)
if res != nil {
c.hbStreams.sendMsg(region, res)
}
return
}
c.removeOperator(op)
}

Although getOperator is protected inside, we need to update Operator after, without any lock. If another goroutine wants to read it, then there will be a data race.

return fmt.Sprintf("%+v", *op)
op.RLock()
defer op.RUnlock()
return jsonString(op)

This comment has been minimized.

@choleraehyq

choleraehyq Aug 17, 2017

Contributor

These defers can be refactor to:

op.RLock()
res := jsonString(op)
op.RUnlock()
return res

Cost of defer in such small functions may be relatively big.

@choleraehyq

choleraehyq Aug 17, 2017

Contributor

These defers can be refactor to:

op.RLock()
res := jsonString(op)
op.RUnlock()
return res

Cost of defer in such small functions may be relatively big.

This comment has been minimized.

@disksing

disksing Aug 18, 2017

Member

operators would not be performance bottleneck, so I prefer readability to performance here.

@disksing

disksing Aug 18, 2017

Member

operators would not be performance bottleneck, so I prefer readability to performance here.

disksing added some commits Aug 21, 2017

@@ -312,3 +313,8 @@ func parseTimestamp(data []byte) (time.Time, error) {
func subTimeByWallClock(after time.Time, before time.Time) time.Duration {
return time.Duration(after.UnixNano() - before.UnixNano())
}
func jsonString(v interface{}) string {
data, _ := json.Marshal(v)

This comment has been minimized.

@siddontang

siddontang Aug 22, 2017

Member

if error, should panic?

@siddontang

siddontang Aug 22, 2017

Member

if error, should panic?

This comment has been minimized.

@disksing

disksing Aug 22, 2017

Member

Only for logging, ok to ignore it, I guess.

@disksing

disksing Aug 22, 2017

Member

Only for logging, ok to ignore it, I guess.

@siddontang

LGTM

@siddontang siddontang merged commit 54e5859 into master Aug 22, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@choleraehyq choleraehyq deleted the disksing/race branch Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment