Fix data races. #2287
Fix data races. #2287
Conversation
|
With these changes, test-unit passes. |
|
test-integration still fails, something to do with use of a buffer. Still investigating. |
build.Dockerfile
Outdated
| @@ -1,8 +1,11 @@ | |||
| FROM golang:1.9-alpine | |||
| FROM golang:1.9 | |||
|
The race detector requires a glibc base image. I left these commits in
incase you wish to run all tests with -race in future. It's probably a good
thing to do, but does slow down the tests.
…On Fri, 20 Oct 2017, 17:37 Emile Vauge, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In build.Dockerfile
<#2287 (comment)>:
> @@ -1,8 +1,11 @@
-FROM golang:1.9-alpine
+FROM golang:1.9
Why did you change this ?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2287 (review)>,
or mute the thread
</~https://github.com/notifications/unsubscribe-auth/AAEo8ys_5g1_QQ0KHkydpgb8t7Dsh-23ks5suMxegaJpZM4QAYCc>
.
|
|
@emilevauge I've pushed what I have. I can't get the integration tests passing locally. The consul_catalogue tests pass, but whatever comes next hangs and it's not obvious from the timeout where. There may be a better approach that the Pipe for getting the output (the original approach requires locking around the buffer though, the Pipe seems cleaner). |
|
I've started making a bit more progress, but there are still a lot of failing tests. staert seems to have an actual issues, ConsulSuite.TestDatastore is the main sticking point right now. |
|
@tcolgate I think it would be better to split this PR into 2: fixes on code and fixes on tests. It would help merging more quickly code races. WDYT? |
|
@emilevauge I'll leave this branch for critical fixes then, and open another PR with the test fixes. |
|
Shouldn't the PR come with a slight change to our build process, namely the activation of the race detector? At least temporarily so that we can see if race detector-enabled builds succeed now? |
|
We can enable the unit tests with these two, but not the integration tests,
which still fail.
|
|
That sounds reasonable to me. |
|
@tcolgate Could you rebase on 1.5 ? |
This fixes a race between dumping the config and setting up the stats.
|
@tcolgate we rebased your PR and kept your fixes on Stats & heathchecks. |
This fixes a race between dumping the config and setting up the stats.