Repository: zeromicro/go-zero
Pull request #3946 (2024-03-01 09:57:49)

fix: BatchError.Add() non thread safe
BatchError.Add()  non thread safe,  `append` operation need  locked.

help to review it. thank!

@kevwan @kesonan 

ch3nnn: example:

```go
fx.Parallel(
	func() {
		_, err := l.svcCtx.xxxRpc.SelectXxx(l.ctx, &Req{})
		if err != nil {
			batchErr.Add(err)
		}
	},
	func() {
		_, err := l.svcCtx.xxxRpc.SelectXxx(l.ctx, &Req{})
		if err != nil {
			batchErr.Add(err)
		}
	},
	func() {
		_, err := l.svcCtx.xxxRpc.SelectXxx(l.ctx, &Req{})
		if err != nil {
			batchErr.Add(err)
		}
	},
)
	
if batchErr.NotNil() {
	return nil, batchErr.Err()
}

```
`batchErr.Err()` error message may be incomplete during concurrency

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/zeromicro/go-zero/pull/3946?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 94.53%. Comparing base [(`5263805`)](https://app.codecov.io/gh/zeromicro/go-zero/commit/5263805b3b557ab8628ba362a57b55a3a84ae790?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro) to head [(`cd351dd`)](https://app.codecov.io/gh/zeromicro/go-zero/pull/3946?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro).

<details><summary>Additional details and impacted files</summary>



| [Files](https://app.codecov.io/gh/zeromicro/go-zero/pull/3946?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro) | Coverage Δ | |
|---|---|---|
| [core/errorx/batcherror.go](https://app.codecov.io/gh/zeromicro/go-zero/pull/3946?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro#diff-Y29yZS9lcnJvcngvYmF0Y2hlcnJvci5nbw==) | `100.00% <100.00%> (ø)` | |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/zeromicro/go-zero/pull/3946/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro)

</details>

Patch:
diff --git a/core/errorx/batcherror.go b/core/errorx/batcherror.go
index 92ae644d..b63867cb 100644
--- a/core/errorx/batcherror.go
+++ b/core/errorx/batcherror.go
@@ -1,10 +1,14 @@
 package errorx
 
-import "bytes"
+import (
+	"bytes"
+	"sync"
+)
 
 type (
 	// A BatchError is an error that can hold multiple errors.
 	BatchError struct {
+		mu   sync.Mutex
 		errs errorArray
 	}
 
@@ -13,6 +17,9 @@ type (
 
 // Add adds errs to be, nil errors are ignored.
 func (be *BatchError) Add(errs ...error) {
+	be.mu.Lock()
+	defer be.mu.Unlock()
+
 	for _, err := range errs {
 		if err != nil {
 			be.errs = append(be.errs, err)


Test patch:
diff --git a/core/errorx/batcherror_test.go b/core/errorx/batcherror_test.go
index ae5c8c3e..3e345343 100644
--- a/core/errorx/batcherror_test.go
+++ b/core/errorx/batcherror_test.go
@@ -3,6 +3,7 @@ package errorx
 import (
 	"errors"
 	"fmt"
+	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -33,7 +34,7 @@ func TestBatchErrorNilFromFunc(t *testing.T) {
 func TestBatchErrorOneError(t *testing.T) {
 	var batch BatchError
 	batch.Add(errors.New(err1))
-	assert.NotNil(t, batch)
+	assert.NotNil(t, batch.Err())
 	assert.Equal(t, err1, batch.Err().Error())
 	assert.True(t, batch.NotNil())
 }
@@ -42,7 +43,26 @@ func TestBatchErrorWithErrors(t *testing.T) {
 	var batch BatchError
 	batch.Add(errors.New(err1))
 	batch.Add(errors.New(err2))
-	assert.NotNil(t, batch)
+	assert.NotNil(t, batch.Err())
 	assert.Equal(t, fmt.Sprintf("%s\n%s", err1, err2), batch.Err().Error())
 	assert.True(t, batch.NotNil())
 }
+
+func TestBatchErrorConcurrentAdd(t *testing.T) {
+	const count = 10000
+	var batch BatchError
+	var wg sync.WaitGroup
+
+	wg.Add(count)
+	for i := 0; i < count; i++ {
+		go func() {
+			defer wg.Done()
+			batch.Add(errors.New(err1))
+		}()
+	}
+	wg.Wait()
+
+	assert.NotNil(t, batch.Err())
+	assert.Equal(t, count, len(batch.errs))
+	assert.True(t, batch.NotNil())
+}