From 041c06dbaa1a1f0b1c1dcbe76dc9b8ffeafa0ffe Mon Sep 17 00:00:00 2001 From: "hang.xiang" Date: Wed, 8 Mar 2023 15:54:08 +0800 Subject: [PATCH 1/3] Copy map instead of assignment in memoryStore.Set --- pkg/cover/store.go | 6 +++++- pkg/cover/store_test.go | 23 +++++++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/pkg/cover/store.go b/pkg/cover/store.go index 0929039e..0eba60af 100644 --- a/pkg/cover/store.go +++ b/pkg/cover/store.go @@ -287,7 +287,11 @@ func (l *memoryStore) Set(services map[string][]string) error { l.mu.Lock() defer l.mu.Unlock() - l.servicesMap = services + newMap := make(map[string][]string) + for k, v := range services { + newMap[k] = append(make([]string, 0), v...) + } + l.servicesMap = newMap return nil } diff --git a/pkg/cover/store_test.go b/pkg/cover/store_test.go index a4898d85..43fada85 100644 --- a/pkg/cover/store_test.go +++ b/pkg/cover/store_test.go @@ -18,10 +18,10 @@ package cover import ( "fmt" + "github.com/stretchr/testify/assert" "os" "testing" - - "github.com/stretchr/testify/assert" + "time" ) func TestLocalStore(t *testing.T) { @@ -130,3 +130,22 @@ func TestFileStoreRemove(t *testing.T) { err = store.Remove("http") assert.Error(t, err, fmt.Errorf("no service found")) } + +// verify issue fix https://github.com/golang/go/issues/56552 +func TestConcurrentRemoval(t *testing.T) { + store, _ := NewFileStore("_svrs_address.txt") + _ = store.Init() + + for i := 0; i < 100; i++ { + _ = store.Add(ServiceUnderTest{ + Name: fmt.Sprintf("test%d", i), + Address: fmt.Sprintf("http://127.0.0.1:890%d", i), + }) + } + + for i := 0; i < 100; i++ { + go store.Remove(fmt.Sprintf("http://127.0.0.1:890%d", i)) + } + + time.Sleep(time.Second * 5) // if no issue then pass. +} From 267535a7c4588eee82ca2a569bed4f3442392524 Mon Sep 17 00:00:00 2001 From: "hang.xiang" Date: Fri, 10 Mar 2023 10:38:15 +0800 Subject: [PATCH 2/3] Fix data inconsistency between fileStore & memStore when calling fileStore.Remove --- pkg/cover/store.go | 47 ++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/pkg/cover/store.go b/pkg/cover/store.go index 0eba60af..467021e8 100644 --- a/pkg/cover/store.go +++ b/pkg/cover/store.go @@ -105,12 +105,15 @@ func (l *fileStore) GetAll() map[string][]string { // Remove the service from the memory store and the file store func (l *fileStore) Remove(addr string) error { + l.mu.Lock() + defer l.mu.Unlock() + err := l.memoryStore.Remove(addr) if err != nil { return err } - return l.Set(l.memoryStore.GetAll()) + return syncToFile(l.persistentFile, l.memoryStore.GetAll()) } // Init cleanup all the registered service information @@ -177,24 +180,7 @@ func (l *fileStore) Set(services map[string][]string) error { return err } - f, err := os.OpenFile(l.persistentFile, os.O_TRUNC|os.O_WRONLY|os.O_CREATE, 0600) - if err != nil { - return err - } - - s := "" - for name, addrs := range services { - for _, addr := range addrs { - s += fmt.Sprintf("%s&%s\n", name, addr) - } - } - - _, err = f.WriteString(s) - if err != nil { - return err - } - - return f.Sync() + return syncToFile(l.persistentFile, services) } func (l *fileStore) appendToFile(s ServiceUnderTest) error { @@ -221,6 +207,27 @@ func split(r rune) bool { return r == '&' } +func syncToFile(persistentFile string, services map[string][]string) error { + f, err := os.OpenFile(persistentFile, os.O_TRUNC|os.O_WRONLY|os.O_CREATE, 0600) + if err != nil { + return err + } + + s := "" + for name, addrs := range services { + for _, addr := range addrs { + s += fmt.Sprintf("%s&%s\n", name, addr) + } + } + + _, err = f.WriteString(s) + if err != nil { + return err + } + + return f.Sync() +} + // memoryStore holds the registered services only into memory type memoryStore struct { mu sync.RWMutex @@ -321,7 +328,7 @@ func (l *memoryStore) Remove(removeAddr string) error { } if !flag { - return fmt.Errorf("no service found") + return fmt.Errorf("no service found: %s", removeAddr) } return nil From 00142a6d4789c53811bfa785240eae74df239b61 Mon Sep 17 00:00:00 2001 From: "hang.xiang" Date: Fri, 10 Mar 2023 11:09:19 +0800 Subject: [PATCH 3/3] Update unit test to be verifiable --- pkg/cover/store_test.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/pkg/cover/store_test.go b/pkg/cover/store_test.go index 43fada85..e1a32acb 100644 --- a/pkg/cover/store_test.go +++ b/pkg/cover/store_test.go @@ -20,8 +20,8 @@ import ( "fmt" "github.com/stretchr/testify/assert" "os" + "sync" "testing" - "time" ) func TestLocalStore(t *testing.T) { @@ -143,9 +143,19 @@ func TestConcurrentRemoval(t *testing.T) { }) } + wg := sync.WaitGroup{} for i := 0; i < 100; i++ { - go store.Remove(fmt.Sprintf("http://127.0.0.1:890%d", i)) + j := i // for loop trap in golang, avoid goroutine uses the same value for i pointer + wg.Add(1) + go func() { + defer wg.Done() + err := store.Remove(fmt.Sprintf("http://127.0.0.1:890%d", j)) + if err != nil { + t.Errorf("fileStore.Remove Error: %v", err) + } + }() } + wg.Wait() - time.Sleep(time.Second * 5) // if no issue then pass. + assert.Equal(t, 0, len(store.GetAll())) }