From 68bdec5b70b063876ef0d9bdebba2a33b3153abd Mon Sep 17 00:00:00 2001 From: Cassondra Foesch Date: Fri, 8 Jun 2018 12:46:56 +0000 Subject: [PATCH 1/4] it is an error to attempt to register a health check twice --- health.go | 27 ++++++++++++++++++++------- health_test.go | 4 ++-- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/health.go b/health.go index 55d55e3..2350c01 100644 --- a/health.go +++ b/health.go @@ -2,6 +2,8 @@ package health import ( "encoding/json" + "errors" + "fmt" "net/http" "runtime" "sync" @@ -9,8 +11,8 @@ import ( ) var ( - mu sync.Mutex - checks []Config + mu sync.Mutex + checkMap = make(map[string]Config) ) const ( @@ -70,14 +72,25 @@ type ( ) // Register registers a check config to be performed. -func Register(c Config) { +func Register(c Config) error { mu.Lock() defer mu.Unlock() if c.Timeout == 0 { c.Timeout = time.Second * 2 } - checks = append(checks, c) + + if c.Name == "" { + return errors.New("health check must have a name to be registered") + } + + if _, ok := checkMap[c.Name]; ok { + return fmt.Errorf("health check %s is already registered", c.Name) + } + + checkMap[c.Name] = c + + return nil } // Handler returns an HTTP handler (http.HandlerFunc). @@ -91,7 +104,7 @@ func HandlerFunc(w http.ResponseWriter, r *http.Request) { defer mu.Unlock() status := statusOK - total := len(checks) + total := len(checkMap) failures := make(map[string]string) resChan := make(chan checkResponse, total) @@ -103,7 +116,7 @@ func HandlerFunc(w http.ResponseWriter, r *http.Request) { wg.Wait() }() - for _, c := range checks { + for _, c := range checkMap { go func(c Config) { defer wg.Done() select { @@ -151,7 +164,7 @@ func Reset() { mu.Lock() defer mu.Unlock() - checks = []Config{} + checkMap = make(map[string]Config) } func newCheck(status string, failures map[string]string) Check { diff --git a/health_test.go b/health_test.go index 0297774..74701ae 100644 --- a/health_test.go +++ b/health_test.go @@ -76,7 +76,7 @@ func TestHealthHandler(t *testing.T) { } Reset() - if len(checks) != 0 { - t.Errorf("checks lenght differes from zero: got %d", len(checks)) + if len(checkMap) != 0 { + t.Errorf("checks lenght differes from zero: got %d", len(checkMap)) } } From 90cb3ad4775a080047ece444de661f5a838d3eca Mon Sep 17 00:00:00 2001 From: Cassondra Foesch Date: Fri, 8 Jun 2018 14:14:13 +0000 Subject: [PATCH 2/4] include tests to ensure that double registration errors are enforced --- health_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/health_test.go b/health_test.go index 74701ae..6da4e9f 100644 --- a/health_test.go +++ b/health_test.go @@ -13,7 +13,63 @@ const ( checkErr = "Failed during RabbitMQ health check" ) +func TestRegisterWithNoName(t *testing.T) { + err := Register(Config{ + Name: "", + Check: func() error { + return nil + }, + }) + if err == nil { + t.Error("health check registration with empty name should return an error, but did not get one") + } +} + +func TestDoubleRegister(t *testing.T) { + Reset() + if len(checkMap) != 0 { + t.Errorf("checks lenght differes from zero: got %d", len(checkMap)) + } + + healthcheckName := "succeed" + + conf := Config{ + Name: healthcheckName, + Check: func() error { + return nil + }, + } + + err := Register(conf) + if err != nil { + // If the first registration failed, then further testing makes no sense. + t.Fatal("the first registration of a health check should not return an error, but got: ", err) + } + + err = Register(conf) + if err == nil { + t.Error("the second registration of a health check should return an error, but did not") + } + + err = Register(Config{ + Name: healthcheckName, + Check: func() error { + // this function is non-trival solely to ensure that the compiler does not get optimized. + if len(checkMap) > 0 { + return nil + } + + return errors.New("no health checks registered") + }, + }) + if err == nil { + t.Error("health check registration with same name different details should still return an error, but did not") + } +} + func TestHealthHandler(t *testing.T) { + Reset() + res := httptest.NewRecorder() req, err := http.NewRequest("GET", "/status", nil) if err != nil { From c893dbaa521441ae1efd7617a7b056b0a21cdefe Mon Sep 17 00:00:00 2001 From: Cassondra Foesch Date: Fri, 8 Jun 2018 14:20:16 +0000 Subject: [PATCH 3/4] simplify and clarify code for tests, comments, and purpose --- health_test.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/health_test.go b/health_test.go index 6da4e9f..df391bf 100644 --- a/health_test.go +++ b/health_test.go @@ -31,7 +31,7 @@ func TestDoubleRegister(t *testing.T) { t.Errorf("checks lenght differes from zero: got %d", len(checkMap)) } - healthcheckName := "succeed" + healthcheckName := "healthcheck" conf := Config{ Name: healthcheckName, @@ -48,22 +48,17 @@ func TestDoubleRegister(t *testing.T) { err = Register(conf) if err == nil { - t.Error("the second registration of a health check should return an error, but did not") + t.Error("the second registration of a health check config should return an error, but did not") } err = Register(Config{ Name: healthcheckName, Check: func() error { - // this function is non-trival solely to ensure that the compiler does not get optimized. - if len(checkMap) > 0 { - return nil - } - - return errors.New("no health checks registered") + return errors.New("health checks registered") }, }) if err == nil { - t.Error("health check registration with same name different details should still return an error, but did not") + t.Error("registration with same name, but different details should still return an error, but did not") } } From 108cbe70d43716d6ec5bbce2c48ae6ea88310fa0 Mon Sep 17 00:00:00 2001 From: Cassondra Foesch Date: Thu, 14 Jun 2018 09:56:13 +0000 Subject: [PATCH 4/4] move lock aquisition to where we absolutely must acquire it, rather than super early. --- health.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/health.go b/health.go index 2350c01..6d4a1aa 100644 --- a/health.go +++ b/health.go @@ -73,9 +73,6 @@ type ( // Register registers a check config to be performed. func Register(c Config) error { - mu.Lock() - defer mu.Unlock() - if c.Timeout == 0 { c.Timeout = time.Second * 2 } @@ -84,6 +81,9 @@ func Register(c Config) error { return errors.New("health check must have a name to be registered") } + mu.Lock() + defer mu.Unlock() + if _, ok := checkMap[c.Name]; ok { return fmt.Errorf("health check %s is already registered", c.Name) }