Repository: go-kratos/kratos
Pull request #1316 (2021-08-10 08:11:12)

fix(config): fix data race
There are some data races in current codes. These data races will break the Github Actions CI testing sometimes. The unit test in this PR could reproduce this issue. This data race will happen when Scan() and Load() in the same time.

This PR adds a lock to the config values map in order to avoid concurrent reads and writes.

data race message:
```
WARNING: DATA RACE
Read at 0x00c0004ac0b8 by goroutine 16:
  github.com/go-kratos/kratos/v2/config.(*reader).Source()
      /Users/windfarer/github/kratos/config/reader.go:58 +0x44
  github.com/go-kratos/kratos/v2/config.(*config).Scan()
      /Users/windfarer/github/kratos/config/config.go:131 +0x5b
  github.com/go-kratos/kratos/v2/config/file.TestMergeDataRace.func1()
      /Users/windfarer/github/kratos/config/file/file_test.go:308 +0x8d

Previous write at 0x00c0004ac0b8 by goroutine 17:
  github.com/go-kratos/kratos/v2/config.(*reader).Merge()
      /Users/windfarer/github/kratos/config/reader.go:49 +0x2fc
  github.com/go-kratos/kratos/v2/config.(*config).Load()
      /Users/windfarer/github/kratos/config/config.go:101 +0x144
  github.com/go-kratos/kratos/v2/config/file.TestMergeDataRace.func2()
      /Users/windfarer/github/kratos/config/file/file_test.go:318 +0x75

WARNING: DATA RACE
Read at 0x00c00040a930 by goroutine 16:
  github.com/go-kratos/kratos/v2/config.convertMap()
      /Users/windfarer/github/kratos/config/reader.go:98 +0xa4
  github.com/go-kratos/kratos/v2/config.convertMap()
      /Users/windfarer/github/kratos/config/reader.go:100 +0x1c9
  github.com/go-kratos/kratos/v2/config.convertMap()
      /Users/windfarer/github/kratos/config/reader.go:100 +0x1c9
  github.com/go-kratos/kratos/v2/config.(*reader).Source()
      /Users/windfarer/github/kratos/config/reader.go:69 +0x6c
  github.com/go-kratos/kratos/v2/config.(*config).Scan()
      /Users/windfarer/github/kratos/config/config.go:131 +0x5b
  github.com/go-kratos/kratos/v2/config/file.TestMergeDataRace.func1()
      /Users/windfarer/github/kratos/config/file/file_test.go:308 +0xcc

Previous write at 0x00c00040a930 by goroutine 17:
  runtime.mapassign_faststr()
      /usr/local/Cellar/go/1.15.5/libexec/src/runtime/map_faststr.go:202 +0x0
  github.com/go-kratos/kratos/v2/config.defaultResolver.func2()
      /Users/windfarer/github/kratos/config/options.go:102 +0x674
  github.com/go-kratos/kratos/v2/config.defaultResolver.func2()
      /Users/windfarer/github/kratos/config/options.go:104 +0x193
  github.com/go-kratos/kratos/v2/config.defaultResolver.func2()
      /Users/windfarer/github/kratos/config/options.go:104 +0x193
  github.com/go-kratos/kratos/v2/config.defaultResolver()
      /Users/windfarer/github/kratos/config/options.go:123 +0x98
  github.com/go-kratos/kratos/v2/config.(*reader).Resolve()
      /Users/windfarer/github/kratos/config/reader.go:73 +0x88
  github.com/go-kratos/kratos/v2/config.(*config).Load()
      /Users/windfarer/github/kratos/config/config.go:112 +0x1fe
  github.com/go-kratos/kratos/v2/config/file.TestMergeDataRace.func2()
      /Users/windfarer/github/kratos/config/file/file_test.go:317 +0xb3

``` 


codecov-commenter: # [Codecov](https://codecov.io/gh/go-kratos/kratos/pull/1316?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) Report
> Merging [#1316](https://codecov.io/gh/go-kratos/kratos/pull/1316?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) (7fb54ea) into [main](https://codecov.io/gh/go-kratos/kratos/commit/9bbf4696d459b67d8ff5624aad6a0f5fccaa3ba3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) (9bbf469) will **increase** coverage by `0.18%`.
> The diff coverage is `100.00%`.

> :exclamation: Current head 7fb54ea differs from pull request most recent head dddbedf. Consider uploading reports for the commit dddbedf to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/go-kratos/kratos/pull/1316/graphs/tree.svg?width=650&height=150&src=pr&token=XwnmBTz78x&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos)](https://codecov.io/gh/go-kratos/kratos/pull/1316?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos)

```diff
@@            Coverage Diff             @@
##             main    #1316      +/-   ##
==========================================
+ Coverage   54.15%   54.34%   +0.18%     
==========================================
  Files          56       56              
  Lines        2661     2672      +11     
==========================================
+ Hits         1441     1452      +11     
  Misses       1078     1078              
  Partials      142      142              
```


| [Impacted Files](https://codecov.io/gh/go-kratos/kratos/pull/1316?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) | Coverage Δ | |
|---|---|---|
| [config/reader.go](https://codecov.io/gh/go-kratos/kratos/pull/1316/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos#diff-Y29uZmlnL3JlYWRlci5nbw==) | `79.16% <100.00%> (+2.69%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/go-kratos/kratos/pull/1316?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/go-kratos/kratos/pull/1316?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos). Last update [9bbf469...dddbedf](https://codecov.io/gh/go-kratos/kratos/pull/1316?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos).


Patch:
diff --git a/config/reader.go b/config/reader.go
index c18b91d3..1410780e 100644
--- a/config/reader.go
+++ b/config/reader.go
@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"strings"
+	"sync"
 
 	"github.com/imdario/mergo"
 	"google.golang.org/protobuf/encoding/protojson"
@@ -23,17 +24,21 @@ type Reader interface {
 type reader struct {
 	opts   options
 	values map[string]interface{}
+	lock   sync.Mutex
 }
 
 func newReader(opts options) Reader {
 	return &reader{
 		opts:   opts,
 		values: make(map[string]interface{}),
+		lock:   sync.Mutex{},
 	}
 }
 
 func (r *reader) Merge(kvs ...*KeyValue) error {
+	r.lock.Lock()
 	merged, err := cloneMap(r.values)
+	r.lock.Unlock()
 	if err != nil {
 		return err
 	}
@@ -46,19 +51,27 @@ func (r *reader) Merge(kvs ...*KeyValue) error {
 			return err
 		}
 	}
+	r.lock.Lock()
 	r.values = merged
+	r.lock.Unlock()
 	return nil
 }
 
 func (r *reader) Value(path string) (Value, bool) {
+	r.lock.Lock()
+	defer r.lock.Unlock()
 	return readValue(r.values, path)
 }
 
 func (r *reader) Source() ([]byte, error) {
+	r.lock.Lock()
+	defer r.lock.Unlock()
 	return marshalJSON(convertMap(r.values))
 }
 
 func (r *reader) Resolve() error {
+	r.lock.Lock()
+	defer r.lock.Unlock()
 	return r.opts.resolver(r.values)
 }
 


Test patch:
diff --git a/config/file/file_test.go b/config/file/file_test.go
index 57fae145..5f9621fa 100644
--- a/config/file/file_test.go
+++ b/config/file/file_test.go
@@ -5,6 +5,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"sync"
 	"testing"
 	"time"
 
@@ -68,14 +69,14 @@ const (
 	}
 }`
 
-//	_testYaml = `
-//Foo:
-//    bar :
-//        - {name: nihao,age: 1}
-//        - {name: nihao,age: 1}
-//
-//
-//`
+	//	_testYaml = `
+	//Foo:
+	//    bar :
+	//        - {name: nihao,age: 1}
+	//        - {name: nihao,age: 1}
+	//
+	//
+	//`
 )
 
 //func TestScan(t *testing.T) {
@@ -288,3 +289,35 @@ func testScan(t *testing.T, c config.Config) {
 	}
 	t.Log(conf)
 }
+
+func TestMergeDataRace(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "test_config.json")
+	defer os.Remove(path)
+	if err := ioutil.WriteFile(path, []byte(_testJSON), 0666); err != nil {
+		t.Error(err)
+	}
+	c := config.New(config.WithSource(
+		NewSource(path),
+	))
+	wg := &sync.WaitGroup{}
+	wg.Add(2)
+	go func() {
+		defer wg.Done()
+		for i := 0; i < 100; i++ {
+			var conf struct{}
+			if err := c.Scan(&conf); err != nil {
+				t.Error(err)
+			}
+		}
+	}()
+
+	go func() {
+		defer wg.Done()
+		for i := 0; i < 100; i++ {
+			if err := c.Load(); err != nil {
+				t.Error(err)
+			}
+		}
+	}()
+	wg.Wait()
+}