Fix data race for max connection limiting in proxy directive. #1438

Merged
merged 2 commits into from Feb 15, 2017

Projects

None yet

2 participants

@augustoroman
Collaborator

The Conns and Unhealthy fields are updated concurrently across all active
requests. Because of this, they must use atomic operations for reads and
writes.

Prior to this change, Conns was incremented atomically, but read unsafely.
Unhealthly was updated & read unsafely. The new test
TestReverseProxyMaxConnLimit exposes this race when run with -race.

Switching to atomic operations makes the race detector happy.

@augustoroman augustoroman Fix data race for max connection limiting in proxy directive.
The Conns and Unhealthy fields are updated concurrently across all active
requests.  Because of this, they must use atomic operations for reads and
writes.

Prior to this change, Conns was incremented atomically, but read unsafely.
Unhealthly was updated & read unsafely.  The new test
TestReverseProxyMaxConnLimit exposes this race when run with -race.

Switching to atomic operations makes the race detector happy.
11ebdd0
@mholt

Thanks for finding this. Just want to double check on a couple things before merging!

+ // This is an int32 so that we can use atomic operations to do concurrent
+ // reads & writes to this value. The default value of 0 indicates that it
+ // is healthy and any non-zero value indicates unhealthy.
+ Unhealthy int32
@mholt
mholt Feb 15, 2017 Owner

Just to confirm, this is int32 so it's not subject to the last bug on this page, right? https://golang.org/pkg/sync/atomic/#pkg-note-BUG - if it is, this would be exposed on ARM systems, for example (Raspberry Pi).

@augustoroman
augustoroman Feb 15, 2017 Collaborator

Oh neat, I hadn't considered that, actually. I just selected int32 since it seemed large enough to hold the single bit value that we are putting in there. :-)

caddyhttp/proxy/proxy_test.go
+ }
+
+ // testComplete := make(chan bool)
+ // go func() {
@mholt
mholt Feb 15, 2017 Owner

Impressive test case. Thank you! I notice a few stray commented lines that made their way in.

@augustoroman
augustoroman Feb 15, 2017 Collaborator

Ooops! Thanks, removed.

@augustoroman augustoroman oops, remove leftover dead code.
7354e50
@mholt
mholt approved these changes Feb 15, 2017 View changes
@mholt mholt merged commit 463c9d9 into mholt:master Feb 15, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@mholt
Owner
mholt commented Feb 15, 2017

Thank you @augustoroman! I really appreciate the initiative you took on this, and the care to write a thorough test. I'm going to invite you to be a collaborator so you can have push rights to the repository. This means you can contribute changes in a branch instead of a fork, and you can help review PRs and help with issues, etc. Once a PR has one or two reviews and is accepted from other collaborators, you can usually merge them (esp. if they're small and have tests).

@augustoroman
Collaborator

Thanks! I'm happy to help where I can, and I appreciate the effort you've already put into making caddy great! :-)

@augustoroman augustoroman deleted the augustoroman:fix-proxy-race branch Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment