check: fix data race on C.status

[davecheney]

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.

[rogpeppe]

This PR should include at least one test case that will fail when called
with the race detector enabled and without this new logic implemented.

I'd actually like to see a whole bunch of concurrency-related tests
in gocheck.

ISTM that there might be a more elegant solution lurking here than
just wrapping status with an atomic operation (there's already a goroutine
around that can potentially guard status updates). I'm not
quite sure what it would look like though. C.Failed is problematic.

[niemeyer]

What does it mean for tests to not pass?

[niemeyer@gopher ..pkg.in/check.v1]% go test
OK: 127 passed
PASS
ok gopkg.in/check.v1 0.210s

[niemeyer@gopher ..pkg.in/check.v1]% go version
go version go1.4 linux/amd64

?

[davecheney]

@niemeyer this was my mistake, the tests do not pass with Go 1.5 (tip), they do pass on the release version of go.

[niemeyer]

Okay, I see. I'll look at that when I have some time.

Regarding the proposed solution, it doesn't smell quite so well to me. It feels like a hack used to silence a very specific problem, but without a proper description of the concurrent behavior we expect from the package as a whole. There are several fields in there which are unprotected.. which of these are okay to touch, and which of them are not? Is c.reason okay to not be covered by the same guarantees of c.status.. why? Also, there is a certain pattern where concurrent functions block on c.done.. can that be used to create some guarantees about not performing concurrent internal maintenance of these fields while the channel is not closed?

If we are to fix this, I'd like to have a more clear line for those details.

[howbazaar]

@niemeyer this bug really does need to be fixed. What solution would be palatable to you? I don't care how the underlying issue is fixed, but it is blocking us. I would much prefer you either fix it a way that you are happy with, or tell us what you would be happy with. Otherwise it feels like we are throwing mud at a wall without a clue.

[niemeyer]

It was week since I last commented on this bug, Tim. It doesn't make sense to ignore it for a week and then come back desperate again.

Then, the comment is a clue, rather than a request for you to throw mud at a wall. If you feel lost, then I will handle it, but you'll need to wait until I can.

Finally, your harsh wording is also inappropriate here, Tim. Please contain yourself so we can keep this friendly.

[howbazaar]

I'm sorry you felt that my previous message had harsh wording, that was certainly not the intent.

We attempted to work around this particular go check issue within our code, and have succeeded in some places, but we do keep running in to it. Hence the break in responding.

I'd rather not have to guess at the clues, but would rather have concrete suggestions to change. I am not really familiar with the gocheck codebase, but I am happy to help and rework the patch so it is acceptable as this is a problem that we are still having.

[niemeyer]

If I had a concrete suggestion, I'd have given you. I gave you the hints that I will use myself if I have to research into the problem and attempt a cleaner fix. That said, Dave's change is simple enough that it doesn't make my life any harder later. If this helps you, I will just apply the hack and fix later.

[niemeyer]

Merged the regretable hack.