From d7d2752be7dcd9b8c4045d9d5ad277076202bfdb Mon Sep 17 00:00:00 2001 From: gengliqi Date: Tue, 25 Jul 2023 19:08:35 +0800 Subject: [PATCH 1/5] fix Signed-off-by: gengliqi --- server/http_handler.go | 13 ++++++----- server/http_handler_test.go | 45 +++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/server/http_handler.go b/server/http_handler.go index 5efbda887bab6..1bd954d1858e3 100644 --- a/server/http_handler.go +++ b/server/http_handler.go @@ -2220,12 +2220,15 @@ func (h labelHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { if len(labels) > 0 { cfg := *config.GetGlobalConfig() - if cfg.Labels == nil { - cfg.Labels = make(map[string]string, len(labels)) - } - for k, v := range labels { - cfg.Labels[k] = v + // Be careful. The key & value of cfg.Labels must not be changed, otherwise data race will happen. + if cfg.Labels != nil { + for k, v := range cfg.Labels { + if _, found := labels[k]; !found { + labels[k] = v + } + } } + cfg.Labels = labels config.StoreGlobalConfig(&cfg) logutil.BgLogger().Info("update server labels", zap.Any("labels", cfg.Labels)) } diff --git a/server/http_handler_test.go b/server/http_handler_test.go index 1253399898e81..467a86f571650 100644 --- a/server/http_handler_test.go +++ b/server/http_handler_test.go @@ -1199,3 +1199,48 @@ func TestSetLabels(t *testing.T) { // reset the global variable config.GetGlobalConfig().Labels = map[string]string{} } + +func TestSetLabelsConcurrentWithGetLabel(t *testing.T) { + ts := createBasicHTTPHandlerTestSuite() + + ts.startServer(t) + defer ts.stopServer(t) + + testUpdateLabels := func(labels, expected map[string]string) { + buffer := bytes.NewBuffer([]byte{}) + require.Nil(t, json.NewEncoder(buffer).Encode(labels)) + resp, err := ts.postStatus("/labels", "application/json", buffer) + require.NoError(t, err) + require.NotNil(t, resp) + defer func() { + require.NoError(t, resp.Body.Close()) + }() + require.Equal(t, http.StatusOK, resp.StatusCode) + newLabels := config.GetGlobalConfig().Labels + require.Equal(t, newLabels, expected) + } + + labels := map[string]string{ + "zone": "us-west-1", + "test": "123", + } + + testUpdateLabels(labels, labels) + + updated := map[string]string{ + "zone": "bj-1", + } + labels["zone"] = "bj-1" + + go func() { + for i := 0; i < 100; i++ { + config.GetGlobalConfig().GetTiKVConfig() + } + }() + for i := 0; i < 100; i++ { + testUpdateLabels(updated, labels) + } + + // reset the global variable + config.GetGlobalConfig().Labels = map[string]string{} +} From 0edf6eea4fb48e9e5ef11f8820fbe6c248d8f686 Mon Sep 17 00:00:00 2001 From: gengliqi Date: Tue, 25 Jul 2023 19:13:23 +0800 Subject: [PATCH 2/5] update Signed-off-by: gengliqi --- server/http_handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/http_handler.go b/server/http_handler.go index 1bd954d1858e3..86a5c9c2fe2e5 100644 --- a/server/http_handler.go +++ b/server/http_handler.go @@ -2220,7 +2220,7 @@ func (h labelHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { if len(labels) > 0 { cfg := *config.GetGlobalConfig() - // Be careful. The key & value of cfg.Labels must not be changed, otherwise data race will happen. + // Be careful of data race. The key & value of cfg.Labels must not be changed. if cfg.Labels != nil { for k, v := range cfg.Labels { if _, found := labels[k]; !found { From 4235850823bbe1bb07a6ea9e05d738bf3348fb18 Mon Sep 17 00:00:00 2001 From: gengliqi Date: Tue, 25 Jul 2023 20:55:48 +0800 Subject: [PATCH 3/5] update test Signed-off-by: gengliqi --- server/http_handler_test.go | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/server/http_handler_test.go b/server/http_handler_test.go index 467a86f571650..03c3a846669f0 100644 --- a/server/http_handler_test.go +++ b/server/http_handler_test.go @@ -24,6 +24,7 @@ import ( "encoding/json" "fmt" "io" + "math/rand" "net" "net/http" "net/http/httptest" @@ -1206,7 +1207,9 @@ func TestSetLabelsConcurrentWithGetLabel(t *testing.T) { ts.startServer(t) defer ts.stopServer(t) - testUpdateLabels := func(labels, expected map[string]string) { + testUpdateLabels := func() { + labels := map[string]string{} + labels["zone"] = fmt.Sprintf("z-%v", rand.Intn(100000)) buffer := bytes.NewBuffer([]byte{}) require.Nil(t, json.NewEncoder(buffer).Encode(labels)) resp, err := ts.postStatus("/labels", "application/json", buffer) @@ -1217,29 +1220,23 @@ func TestSetLabelsConcurrentWithGetLabel(t *testing.T) { }() require.Equal(t, http.StatusOK, resp.StatusCode) newLabels := config.GetGlobalConfig().Labels - require.Equal(t, newLabels, expected) - } - - labels := map[string]string{ - "zone": "us-west-1", - "test": "123", - } - - testUpdateLabels(labels, labels) - - updated := map[string]string{ - "zone": "bj-1", + require.Equal(t, newLabels, labels) } - labels["zone"] = "bj-1" - + done := make(chan struct{}) go func() { - for i := 0; i < 100; i++ { - config.GetGlobalConfig().GetTiKVConfig() + for { + select { + case <-done: + return + default: + config.GetGlobalConfig().GetTiKVConfig() + } } }() for i := 0; i < 100; i++ { - testUpdateLabels(updated, labels) + testUpdateLabels() } + close(done) // reset the global variable config.GetGlobalConfig().Labels = map[string]string{} From 0869452a015e2bae18fdb52ecb0184d5652953b2 Mon Sep 17 00:00:00 2001 From: gengliqi Date: Wed, 26 Jul 2023 13:41:42 +0800 Subject: [PATCH 4/5] fix unstable test Signed-off-by: gengliqi --- server/http_handler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/http_handler_test.go b/server/http_handler_test.go index 03c3a846669f0..6ff2dc96daa0e 100644 --- a/server/http_handler_test.go +++ b/server/http_handler_test.go @@ -1236,7 +1236,7 @@ func TestSetLabelsConcurrentWithGetLabel(t *testing.T) { for i := 0; i < 100; i++ { testUpdateLabels() } - close(done) + done <- struct{}{} // reset the global variable config.GetGlobalConfig().Labels = map[string]string{} From 45b30f4a9ef681dbce6a532a2ccbf66121d62259 Mon Sep 17 00:00:00 2001 From: gengliqi Date: Wed, 26 Jul 2023 15:32:29 +0800 Subject: [PATCH 5/5] address comments Signed-off-by: gengliqi --- server/http_handler_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/server/http_handler_test.go b/server/http_handler_test.go index 6ff2dc96daa0e..e47f8ce0fc87a 100644 --- a/server/http_handler_test.go +++ b/server/http_handler_test.go @@ -1198,7 +1198,9 @@ func TestSetLabels(t *testing.T) { testUpdateLabels(updated, labels) // reset the global variable - config.GetGlobalConfig().Labels = map[string]string{} + config.UpdateGlobal(func(conf *config.Config) { + conf.Labels = map[string]string{} + }) } func TestSetLabelsConcurrentWithGetLabel(t *testing.T) { @@ -1236,8 +1238,10 @@ func TestSetLabelsConcurrentWithGetLabel(t *testing.T) { for i := 0; i < 100; i++ { testUpdateLabels() } - done <- struct{}{} + close(done) // reset the global variable - config.GetGlobalConfig().Labels = map[string]string{} + config.UpdateGlobal(func(conf *config.Config) { + conf.Labels = map[string]string{} + }) }