Skip to content

Conversation

juliusv
Copy link
Member

@juliusv juliusv commented Jan 31, 2015

The RLock already needs to be acquired when reading r.dimHashesByName.

This fixes #61

@beorn7 @stapelberg

The RLock already needs to be acquired when reading r.dimHashesByName.

This fixes #61
@grobie
Copy link
Member

grobie commented Jan 31, 2015

What about adding the referenced example to the test suite to ensure the advertised behavior of concurrent/late registration of metrics work? This will also allow to check for race conditions with the test suite.

@juliusv
Copy link
Member Author

juliusv commented Jan 31, 2015

@grobie I could add a test case, but don't know a way to test the race directly. I'd have to add a separate Makefile step to run that specific test with the race detector enabled. Is that worth it? I think if we have races again, it would be much more likely a different race that that particular test wouldn't catch anyways, and it's not possible to run the whole test suite with -race enabled because it uses too much RAM.

@beorn7
Copy link
Member

beorn7 commented Jan 31, 2015

👍

@beorn7
Copy link
Member

beorn7 commented Jan 31, 2015

@grobie @juliusv In principle, think it makes sense to add a test for it, and then mention in the doc string that it can be run with -race to test for races. We have a similar case in prometheus/prometheus/storage/local. The test would be able to detect some other races, too, especially if it's a bigger fuzz test of some kind.

We can do that later, though.

beorn7 added a commit that referenced this pull request Jan 31, 2015
Fix race condition in writePB().
@beorn7 beorn7 merged commit aa1328d into master Jan 31, 2015
@beorn7 beorn7 deleted the fix-writepb-race branch January 31, 2015 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: data race in Register() vs. ServeHTTP()

3 participants