Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cmd/go: test -race -covermode=count correctly detects cover races #12118

Closed
jmhodges opened this issue Aug 12, 2015 · 9 comments
Closed

cmd/go: test -race -covermode=count correctly detects cover races #12118

jmhodges opened this issue Aug 12, 2015 · 9 comments
Labels
FrozenDueToAge
Milestone
Go1.6Early

Comments

@jmhodges
Copy link
Contributor

Looking at https://travis-ci.org/jmhodges/boulder/builds/75187815, the first DATA RACE section has

WARNING: DATA RACE
Read by goroutine 6:
  github.com/jmhodges/boulder/log.TestConstruction()
      /home/travis/gopath/src/github.com/jmhodges/boulder/log/audit-logger_test.go:216 +0x1f4
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:456 +0xdc

Previous write by goroutine 8:
  github.com/jmhodges/boulder/log.Dial()
      /home/travis/gopath/src/github.com/jmhodges/boulder/log/audit-logger_test.go:216 +0x26b
  github.com/jmhodges/boulder/log.initializeAuditLogger()
      github.com/jmhodges/boulder/log/_test/_obj_test/audit-logger.go:110 +0x217

Notice that both filepaths and line numbers are the same, but the functions referenced are different.

Also, the code at audit-logger_test.go:216 (not my filename choice; don't hate) is:

audit, err := NewAuditLogger(writer, stats)

which does not deal directly log.Dial or log.initializeAuditLogger. You can find this code, currently, at https://github.com/letsencrypt/boulder/blob/2d92b1f348719ab096292308437b88ec0bb31979/log/audit-logger_test.go#L216
@jmhodges
Copy link
Contributor Author

Note: I've not been able to get the OS X Go 1.5rc1 race detector to raise these race conditions, yet. Only the TravisCI one that is on linux. I've not had a chance to repro on a real linux machine directly.

@ianlancetaylor ianlancetaylor added this to the Go1.5Maybe milestone Aug 12, 2015
@jmhodges
Copy link
Contributor Author

Okay, the places I found that were obviously races in the boulder/log package have not fixed those DATA RACEs and the busted constructed stack traces have made it difficult to find the rest of the races. The attempted fixes can be found in this branch: https://github.com/jmhodges/boulder/tree/auditlogger_race/log

It's an unusual library, but one that is real and is being deployed to a production-like setting. Sorry for its weirdness.

@jmhodges
Copy link
Contributor Author

Also, this occurs in tip on TravisCI at go version devel +58035ec Tue Aug 11 20:46:22 2015 +0000 linux/amd64. See https://travis-ci.org/jmhodges/boulder/builds/75205905

@jmhodges jmhodges changed the title cmd/go: possible miscalculations in the race detector stack traces in Go 1.5rc1 cmd/go: miscalculations in the race detector in Go 1.5rc1 Aug 12, 2015
@aclements
Copy link
Member

@dvyukov

@dvyukov
Copy link
Member

dvyukov commented Aug 12, 2015

You are using -covermode=count, these are races on coverage counters.
-race -covermode=count combination is doomed to fail, @robpike can we produce some kind of a warning for this combination? Fail build?

@rsc
Copy link
Contributor

rsc commented Aug 12, 2015

If you ask for go test -race -covermode=count, then that's what you get. There is even a test case for this in cmd/go. Don't do that. There's no bug for Go 1.5 here.

@rsc rsc changed the title cmd/go: miscalculations in the race detector in Go 1.5rc1 cmd/go: test -race -covermode=count correctly detects cover races Aug 12, 2015
@rsc rsc modified the milestones: Go1.6Early, Go1.5Maybe Aug 12, 2015
@jmhodges
Copy link
Contributor Author

Okay, cool. I hadn't thought about those being related.

jmhodges added a commit to letsencrypt/boulder that referenced this issue Aug 13, 2015
@jmhodges
separate race and coverage testing in TravisCI
0656421 
There are races in the count cover mode and there is an issue for it:
golang/go#12118.
@jmhodges jmhodges mentioned this issue Aug 13, 2015
Closed
jmhodges added a commit to letsencrypt/boulder that referenced this issue Aug 13, 2015
@jmhodges
separate race and coverage testing in TravisCI
43cf49a 
There are races in the count cover mode and that makes spurious-to-us looking race conditions crop up for things that are not races. There is an issue for it: golang/go#12118.
jmhodges added a commit to letsencrypt/boulder that referenced this issue Aug 13, 2015
@jmhodges
use the default, non-race covermode in TravisCI
b39ca81 
The count covermode is racy and was causing spurious-to-us looking (but not
really) race detections in the tests. See
golang/go#12118
@jmhodges jmhodges mentioned this issue Aug 13, 2015
Merged
jmhodges added a commit to letsencrypt/boulder that referenced this issue Aug 13, 2015
@jmhodges
use the default, not racy covermode in TravisCI
25ba6e0 
The count covermode is racy and was causing spurious-to-us looking (but not
really) race detections in the tests. See
golang/go#12118
@robpike
Copy link
Contributor

robpike commented Aug 17, 2015

This is what covermode -atomic is for.

@robpike
Copy link
Contributor

robpike commented Aug 17, 2015

Not an issue. The title even says, "correctly". :)

@robpike robpike closed this as completed Aug 17, 2015
@golang golang locked and limited conversation to collaborators Aug 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.6Early
Development

No branches or pull requests
7 participants
@jmhodges @rsc @dvyukov @aclements @ianlancetaylor @robpike @gopherbot