middleware/proxy: Make Unhealthy a pointer (#615)

Pointer updates are atomic so drop the sync.RWMutex as it is not needed
anymore. This also fixes the race introduced with dfc71df (although I
believe this is the first time we properly tested that code path).

6 files changed, 28 insertions, 24 deletions

diff --git a/middleware/proxy/google.go b/middleware/proxy/google.go
index dc83755ad..09fb13e2f 100644
--- a/middleware/proxy/google.go
+++ b/middleware/proxy/google.go
@@ -218,11 +218,11 @@ func newUpstream(hosts []string, old *staticUpstream) Upstream {
 			Conns:       0,
 			Fails:       0,
 			FailTimeout: upstream.FailTimeout,
-			Unhealthy:   false,
+			Unhealthy:   newBool(),
 
 			CheckDown: func(upstream *staticUpstream) UpstreamHostDownFunc {
 				return func(uh *UpstreamHost) bool {
-					if uh.Unhealthy {
+					if *uh.Unhealthy {
 						return true
 					}
 
diff --git a/middleware/proxy/lookup.go b/middleware/proxy/lookup.go
index e97741fb5..e81b5f808 100644
--- a/middleware/proxy/lookup.go
+++ b/middleware/proxy/lookup.go
@@ -38,10 +38,10 @@ func NewLookupWithOption(hosts []string, opts Options) Proxy {
 			Fails:       0,
 			FailTimeout: upstream.FailTimeout,
 
-			Unhealthy: false,
+			Unhealthy: newBool(),
 			CheckDown: func(upstream *staticUpstream) UpstreamHostDownFunc {
 				return func(uh *UpstreamHost) bool {
-					if uh.Unhealthy {
+					if *uh.Unhealthy {
 						return true
 					}
 					fails := atomic.LoadInt32(&uh.Fails)
diff --git a/middleware/proxy/policy_test.go b/middleware/proxy/policy_test.go
index 8f4f1f792..e55fceadc 100644
--- a/middleware/proxy/policy_test.go
+++ b/middleware/proxy/policy_test.go
@@ -28,13 +28,16 @@ func (r *customPolicy) Select(pool HostPool) *UpstreamHost {
 func testPool() HostPool {
 	pool := []*UpstreamHost{
 		{
-			Name: workableServer.URL, // this should resolve (healthcheck test)
+			Name:      workableServer.URL, // this should resolve (healthcheck test)
+			Unhealthy: newBool(),
 		},
 		{
-			Name: "http://shouldnot.resolve", // this shouldn't
+			Name:      "http://shouldnot.resolve", // this shouldn't
+			Unhealthy: newBool(),
 		},
 		{
-			Name: "http://C",
+			Name:      "http://C",
+			Unhealthy: newBool(),
 		},
 	}
 	return HostPool(pool)
@@ -54,7 +57,7 @@ func TestRoundRobinPolicy(t *testing.T) {
 		t.Error("Expected second round robin host to be third host in the pool.")
 	}
 	// mark host as down
-	pool[0].Unhealthy = true
+	*pool[0].Unhealthy = true
 	h = rrPolicy.Select(pool)
 	if h != pool[1] {
 		t.Error("Expected third round robin host to be first host in the pool.")
diff --git a/middleware/proxy/proxy.go b/middleware/proxy/proxy.go
index ce8b99d83..ca7b8daa0 100644
--- a/middleware/proxy/proxy.go
+++ b/middleware/proxy/proxy.go
@@ -3,7 +3,6 @@ package proxy
 
 import (
 	"errors"
-	"sync"
 	"sync/atomic"
 	"time"
 
@@ -57,10 +56,9 @@ type UpstreamHost struct {
 	Name              string // IP address (and port) of this upstream host
 	Fails             int32
 	FailTimeout       time.Duration
-	Unhealthy         bool
+	Unhealthy         *bool
 	CheckDown         UpstreamHostDownFunc
 	WithoutPathPrefix string
-	checkMu           sync.Mutex
 }
 
 // Down checks whether the upstream host is down or not.
@@ -70,7 +68,7 @@ func (uh *UpstreamHost) Down() bool {
 	if uh.CheckDown == nil {
 		// Default settings
 		fails := atomic.LoadInt32(&uh.Fails)
-		return uh.Unhealthy || fails > 0
+		return *uh.Unhealthy || fails > 0
 	}
 	return uh.CheckDown(uh)
 }
diff --git a/middleware/proxy/upstream.go b/middleware/proxy/upstream.go
index c595976bc..a69ebe275 100644
--- a/middleware/proxy/upstream.go
+++ b/middleware/proxy/upstream.go
@@ -84,11 +84,11 @@ func NewStaticUpstreams(c *caddyfile.Dispenser) ([]Upstream, error) {
 				Conns:       0,
 				Fails:       0,
 				FailTimeout: upstream.FailTimeout,
-				Unhealthy:   false,
+				Unhealthy:   newBool(),
 
 				CheckDown: func(upstream *staticUpstream) UpstreamHostDownFunc {
 					return func(uh *UpstreamHost) bool {
-						if uh.Unhealthy {
+						if *uh.Unhealthy {
 							return true
 						}
 
@@ -251,22 +251,19 @@ func (u *staticUpstream) healthCheck() {
 
 		hostURL := "http://" + net.JoinHostPort(checkHostName, checkPort) + u.HealthCheck.Path
 
-		host.checkMu.Lock()
-		defer host.checkMu.Unlock()
-
 		if r, err := http.Get(hostURL); err == nil {
 			io.Copy(ioutil.Discard, r.Body)
 			r.Body.Close()
 			if r.StatusCode < 200 || r.StatusCode >= 400 {
 				log.Printf("[WARNING] Health check URL %s returned HTTP code %d\n",
 					hostURL, r.StatusCode)
-				host.Unhealthy = true
+				*host.Unhealthy = true
 			} else {
-				host.Unhealthy = false
+				*host.Unhealthy = false
 			}
 		} else {
 			log.Printf("[WARNING] Health check probe failed: %v\n", err)
-			host.Unhealthy = true
+			*host.Unhealthy = true
 		}
 	}
 }
@@ -341,3 +338,9 @@ func (u *staticUpstream) IsAllowedDomain(name string) bool {
 }
 
 func (u *staticUpstream) Exchanger() Exchanger { return u.ex }
+
+func newBool() *bool {
+	b := new(bool)
+	*b = false
+	return b
+}
diff --git a/middleware/proxy/upstream_test.go b/middleware/proxy/upstream_test.go
index 587d96994..fbdb0ce4a 100644
--- a/middleware/proxy/upstream_test.go
+++ b/middleware/proxy/upstream_test.go
@@ -42,13 +42,13 @@ func TestSelect(t *testing.T) {
 		FailTimeout: 10 * time.Second,
 		MaxFails:    1,
 	}
-	upstream.Hosts[0].Unhealthy = true
-	upstream.Hosts[1].Unhealthy = true
-	upstream.Hosts[2].Unhealthy = true
+	*upstream.Hosts[0].Unhealthy = true
+	*upstream.Hosts[1].Unhealthy = true
+	*upstream.Hosts[2].Unhealthy = true
 	if h := upstream.Select(); h != nil {
 		t.Error("Expected select to return nil as all host are down")
 	}
-	upstream.Hosts[2].Unhealthy = false
+	*upstream.Hosts[2].Unhealthy = false
 	if h := upstream.Select(); h == nil {
 		t.Error("Expected select to not return nil")
 	}