Fix health race (#645)

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

This reverts commit acbf522cebdcd53c26d153c1d9267d709ba75f64.

* middleware/proxy: add proper locking

This add the proper locking around `Unhealthy`.

6 files changed, 26 insertions, 28 deletions

diff --git a/middleware/proxy/google.go b/middleware/proxy/google.go
index 09fb13e2f..dc83755ad 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:   newBool(),
+			Unhealthy:   false,
 
 			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 e81b5f808..e97741fb5 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: newBool(),
+			Unhealthy: false,
 			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 e55fceadc..8f4f1f792 100644
--- a/middleware/proxy/policy_test.go
+++ b/middleware/proxy/policy_test.go
@@ -28,16 +28,13 @@ func (r *customPolicy) Select(pool HostPool) *UpstreamHost {
 func testPool() HostPool {
 	pool := []*UpstreamHost{
 		{
-			Name:      workableServer.URL, // this should resolve (healthcheck test)
-			Unhealthy: newBool(),
+			Name: workableServer.URL, // this should resolve (healthcheck test)
 		},
 		{
-			Name:      "http://shouldnot.resolve", // this shouldn't
-			Unhealthy: newBool(),
+			Name: "http://shouldnot.resolve", // this shouldn't
 		},
 		{
-			Name:      "http://C",
-			Unhealthy: newBool(),
+			Name: "http://C",
 		},
 	}
 	return HostPool(pool)
@@ -57,7 +54,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 ca7b8daa0..ce8b99d83 100644
--- a/middleware/proxy/proxy.go
+++ b/middleware/proxy/proxy.go
@@ -3,6 +3,7 @@ package proxy
 
 import (
 	"errors"
+	"sync"
 	"sync/atomic"
 	"time"
 
@@ -56,9 +57,10 @@ 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.
@@ -68,7 +70,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 a69ebe275..278b08d0f 100644
--- a/middleware/proxy/upstream.go
+++ b/middleware/proxy/upstream.go
@@ -84,11 +84,13 @@ func NewStaticUpstreams(c *caddyfile.Dispenser) ([]Upstream, error) {
 				Conns:       0,
 				Fails:       0,
 				FailTimeout: upstream.FailTimeout,
-				Unhealthy:   newBool(),
+				Unhealthy:   false,
 
 				CheckDown: func(upstream *staticUpstream) UpstreamHostDownFunc {
 					return func(uh *UpstreamHost) bool {
-						if *uh.Unhealthy {
+						uh.checkMu.Lock()
+						defer uh.checkMu.Unlock()
+						if uh.Unhealthy {
 							return true
 						}
 
@@ -251,19 +253,22 @@ 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
 		}
 	}
 }
@@ -338,9 +343,3 @@ 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 fbdb0ce4a..587d96994 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")
 	}