/check

Attempting to make check.C safe for concurrent use #35

Open
wants to merge 2 commits into
from
+15 −0

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@cbroglie @afinkenstadt @niemeyer
@cbroglie

cbroglie commented Mar 23, 2015

This fixes some known issues, there may be others still
@cbroglie
Attempting to make check.C safe for concurrent use 
This fixes some known issues, there may be others still
7eaf4fb
@cbroglie

cbroglie commented Mar 23, 2015

Fixes issues like these, found by the go race detector:

WARNING: DATA RACE
Write by goroutine 570:
  gopkg.in/check%2ev1.(*C).internalCheck()
      .../Godeps/_workspace/src/gopkg.in/check.v1/helpers.go:227 +0x124c
  gopkg.in/check%2ev1.(*C).Assert()
      .../Godeps/_workspace/src/gopkg.in/check.v1/helpers.go:173 +0x9a

Previous write by goroutine 569:
  gopkg.in/check%2ev1.(*C).internalCheck()
      .../Godeps/_workspace/src/gopkg.in/check.v1/helpers.go:227 +0x124c
  gopkg.in/check%2ev1.(*C).Assert()
      .../Godeps/_workspace/src/gopkg.in/check.v1/helpers.go:173 +0x9a
@afinkenstadt

afinkenstadt commented Mar 23, 2015

👍
@niemeyer
Contributor

niemeyer commented Jun 10, 2015

I'm happy to merge something along these lines, but can we please have a simple mutex instead of a RW one? These are all extremely fast operations which don't benefit from concurrency.

davecheney added a commit to juju/check that referenced this pull request Jun 17, 2015

@davecheney
check: fix data race on C.status 
Fixes #43
Updates #35

This PR removes a data race on C.status if c.Error or c.Assert fails in a goroutine spawned inside a test suite.

Renaming the variable to `C._status` is regretable, but made the change to the code accessing the variable via helpers smaller.

Tests were _not_ run as they do not pass on recent versions of Go. This fix was verified by asserting that LP 1463643 was resolved.
758efea

@davecheney davecheney referenced this pull request Jun 17, 2015

Merged

check: fix data race on C.status #44

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