Avoid allocating a new slice on every Append (#9)

Currently, appending a non-nil error to a multierror always copies the underlying slice, since there may be multiple goroutines appending to the same multierr,
```go
merr := multierr.Append(errors.New("initial"), errors.New("multierr"))
go func() {
  err1 = multierr.Append(merr, errors.New("1"))
  [..]
}()
go func() {
  err2 = multierr.Append(merr, errors.New("2"))
  [..]
}()
```

However, the common use case is a standard for loop:
```go
var merr error
for _, v := range values {
  merr = multierr.Append(merr, processValue(v))
}
return merr
```

Since there is only a single resulting slice, we don't need to create a full copy on every `Append`. Instead, we track whether we have modified the underlying slice from `Append` already using an atomic boolean, and only create a copy if the underlying slice has been modified.

error.go

@@ -27,6 +27,8 @@ import (
 	"io"
 	"strings"
 	"sync"
+
+	"go.uber.org/atomic"
 )
 
 var (
@@ -68,13 +70,12 @@ var _bufferPool = sync.Pool{
 //
 // multiError formats to a semi-colon delimited list of error messages with
 // %v and with a more readable multi-line format with %+v.
-type multiError []error
-
-func (merr multiError) String() string {
-	return merr.Error()
+type multiError struct {
+	copyNeeded atomic.Bool
+	errors     []error
 }
 
-func (merr multiError) Error() string {
+func (merr *multiError) Error() string {
 	buff := _bufferPool.Get().(*bytes.Buffer)
 	buff.Reset()
 
@@ -85,7 +86,7 @@ func (merr multiError) Error() string {
 	return result
 }
 
-func (merr multiError) Format(f fmt.State, c rune) {
+func (merr *multiError) Format(f fmt.State, c rune) {
 	if c == 'v' && f.Flag('+') {
 		merr.writeMultiline(f)
 	} else {
@@ -93,9 +94,9 @@ func (merr multiError) Format(f fmt.State, c rune) {
 	}
 }
 
-func (merr multiError) writeSingleline(w io.Writer) {
+func (merr *multiError) writeSingleline(w io.Writer) {
 	first := true
-	for _, item := range merr {
+	for _, item := range merr.errors {
 		if first {
 			first = false
 		} else {
@@ -105,9 +106,9 @@ func (merr multiError) writeSingleline(w io.Writer) {
 	}
 }
 
-func (merr multiError) writeMultiline(w io.Writer) {
+func (merr *multiError) writeMultiline(w io.Writer) {
 	w.Write(_multilinePrefix)
-	for _, item := range merr {
+	for _, item := range merr.errors {
 		w.Write(_multilineSeparator)
 		writePrefixLine(w, _multilineIndent, fmt.Sprintf("%+v", item))
 	}
@@ -164,8 +165,8 @@ func inspect(errors []error) (res inspectResult) {
 			res.FirstErrorIdx = i
 		}
 
-		if merr, ok := err.(multiError); ok {
-			res.Capacity += len(merr)
+		if merr, ok := err.(*multiError); ok {
+			res.Capacity += len(merr.errors)
 			res.ContainsMultiError = true
 		} else {
 			res.Capacity++
@@ -186,23 +187,24 @@ func fromSlice(errors []error) error {
 	case len(errors):
 		if !res.ContainsMultiError {
 			// already flat
-			return multiError(errors)
+			return &multiError{errors: errors}
 		}
 	}
 
-	merr := make(multiError, 0, res.Capacity)
+	nonNilErrs := make([]error, 0, res.Capacity)
 	for _, err := range errors[res.FirstErrorIdx:] {
 		if err == nil {
 			continue
 		}
 
-		if nested, ok := err.(multiError); ok {
-			merr = append(merr, nested...)
+		if nested, ok := err.(*multiError); ok {
+			nonNilErrs = append(nonNilErrs, nested.errors...)
 		} else {
-			merr = append(merr, err)
+			nonNilErrs = append(nonNilErrs, err)
 		}
 	}
-	return merr
+
+	return &multiError{errors: nonNilErrs}
 }
 
 // Combine combines the passed errors into a single error.
@@ -263,10 +265,15 @@ func Append(left error, right error) error {
 		return left
 	}
 
-	if _, ok := right.(multiError); !ok {
-		if _, ok := left.(multiError); !ok {
+	if _, ok := right.(*multiError); !ok {
+		if l, ok := left.(*multiError); ok && !l.copyNeeded.Swap(true) {
+			// Common case where the error on the left is constantly being
+			// appended to.
+			errs := append(l.errors, right)
+			return &multiError{errors: errs}
+		} else if !ok {
 			// Both errors are single errors.
-			return multiError{left, right}
+			return &multiError{errors: []error{left, right}}
 		}
 	}
 

error_test.go

@@ -24,6 +24,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -54,6 +55,10 @@ func appendN(initial, err error, n int) error {
 	return errs
 }
 
+func newMultiErr(errors ...error) error {
+	return &multiError{errors: errors}
+}
+
 func TestCombine(t *testing.T) {
 	tests := []struct {
 		giveErrors     []error
@@ -73,15 +78,15 @@ func TestCombine(t *testing.T) {
 			giveErrors: []error{
 				errors.New("foo"),
 				nil,
-				multiError{
+				newMultiErr(
 					errors.New("bar"),
-				},
+				),
 				nil,
 			},
-			wantError: multiError{
+			wantError: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
-			},
+			),
 			wantMultiline: "the following errors occurred:\n" +
 				" -  foo\n" +
 				" -  bar",
@@ -90,14 +95,14 @@ func TestCombine(t *testing.T) {
 		{
 			giveErrors: []error{
 				errors.New("foo"),
-				multiError{
+				newMultiErr(
 					errors.New("bar"),
+				),
 			},
-			},
-			wantError: multiError{
+			wantError: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
-			},
+			),
 			wantMultiline: "the following errors occurred:\n" +
 				" -  foo\n" +
 				" -  bar",
@@ -114,10 +119,10 @@ func TestCombine(t *testing.T) {
 				errors.New("foo"),
 				errors.New("bar"),
 			},
-			wantError: multiError{
+			wantError: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
-			},
+			),
 			wantMultiline: "the following errors occurred:\n" +
 				" -  foo\n" +
 				" -  bar",
@@ -129,11 +134,11 @@ func TestCombine(t *testing.T) {
 				errors.New("multi\n  line\nerror message"),
 				errors.New("single line error message"),
 			},
-			wantError: multiError{
+			wantError: newMultiErr(
 				errors.New("great sadness"),
 				errors.New("multi\n  line\nerror message"),
 				errors.New("single line error message"),
-			},
+			),
 			wantMultiline: "the following errors occurred:\n" +
 				" -  great sadness\n" +
 				" -  multi\n" +
@@ -147,18 +152,18 @@ func TestCombine(t *testing.T) {
 		{
 			giveErrors: []error{
 				errors.New("foo"),
-				multiError{
+				newMultiErr(
 					errors.New("bar"),
 					errors.New("baz"),
-				},
+				),
 				errors.New("qux"),
 			},
-			wantError: multiError{
+			wantError: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
 				errors.New("baz"),
 				errors.New("qux"),
-			},
+			),
 			wantMultiline: "the following errors occurred:\n" +
 				" -  foo\n" +
 				" -  bar\n" +
@@ -170,15 +175,15 @@ func TestCombine(t *testing.T) {
 			giveErrors: []error{
 				errors.New("foo"),
 				nil,
-				multiError{
+				newMultiErr(
 					errors.New("bar"),
-				},
+				),
 				nil,
 			},
-			wantError: multiError{
+			wantError: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
-			},
+			),
 			wantMultiline: "the following errors occurred:\n" +
 				" -  foo\n" +
 				" -  bar",
@@ -187,14 +192,14 @@ func TestCombine(t *testing.T) {
 		{
 			giveErrors: []error{
 				errors.New("foo"),
-				multiError{
+				newMultiErr(
 					errors.New("bar"),
+				),
 			},
-			},
-			wantError: multiError{
+			wantError: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
-			},
+			),
 			wantMultiline: "the following errors occurred:\n" +
 				" -  foo\n" +
 				" -  bar",
@@ -206,11 +211,11 @@ func TestCombine(t *testing.T) {
 				richFormatError{},
 				errors.New("bar"),
 			},
-			wantError: multiError{
+			wantError: newMultiErr(
 				errors.New("foo"),
 				richFormatError{},
 				errors.New("bar"),
-			},
+			),
 			wantMultiline: "the following errors occurred:\n" +
 				" -  foo\n" +
 				" -  multiline\n" +
@@ -277,46 +282,46 @@ func TestAppend(t *testing.T) {
 		{
 			left:  errors.New("foo"),
 			right: errors.New("bar"),
-			want: multiError{
+			want: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
-			},
+			),
 		},
 		{
-			left: multiError{
+			left: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
-			},
+			),
 			right: errors.New("baz"),
-			want: multiError{
+			want: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
 				errors.New("baz"),
-			},
+			),
 		},
 		{
 			left: errors.New("baz"),
-			right: multiError{
+			right: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
-			},
-			want: multiError{
+			),
+			want: newMultiErr(
 				errors.New("baz"),
 				errors.New("foo"),
 				errors.New("bar"),
-			},
+			),
 		},
 		{
-			left: multiError{
+			left: newMultiErr(
 				errors.New("foo"),
-			},
-			right: multiError{
+			),
+			right: newMultiErr(
 				errors.New("bar"),
-			},
-			want: multiError{
+			),
+			want: newMultiErr(
 				errors.New("foo"),
 				errors.New("bar"),
-			},
+			),
 		},
 	}
 
@@ -325,20 +330,40 @@ func TestAppend(t *testing.T) {
 	}
 }
 
-func TestAppendDoesNotModify(t *testing.T) {
-	createInitial := func() error {
+func createMultiErrWithCapacity() error {
 	// Create a multiError that has capacity for more errors so Append will
 	// modify the underlying array that may be shared.
-		return appendN(nil, errors.New("initial"), 50)
+	return appendN(nil, errors.New("append"), 50)
 }
 
-	initial := createInitial()
+func TestAppendDoesNotModify(t *testing.T) {
+	initial := createMultiErrWithCapacity()
 	err1 := Append(initial, errors.New("err1"))
 	err2 := Append(initial, errors.New("err2"))
 
-	// initial should not have been modified.
-	assert.Equal(t, createInitial(), initial, "Initial should not be modified")
+	// Make sure the error messages match, since we do modify the copyNeeded
+	// atomic, the values cannot be compared.
+	assert.EqualError(t, initial, createMultiErrWithCapacity().Error(), "Initial should not be modified")
 
-	assert.Equal(t, Append(createInitial(), errors.New("err1")), err1)
-	assert.Equal(t, Append(createInitial(), errors.New("err2")), err2)
+	assert.EqualError(t, err1, Append(createMultiErrWithCapacity(), errors.New("err1")).Error())
+	assert.EqualError(t, err2, Append(createMultiErrWithCapacity(), errors.New("err2")).Error())
+}
+
+func TestAppendRace(t *testing.T) {
+	initial := createMultiErrWithCapacity()
+
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+
+			err := initial
+			for j := 0; j < 10; j++ {
+				err = Append(err, errors.New("err"))
+			}
+		}()
+	}
+
+	wg.Wait()
 }

glide.lock

@@ -1,16 +1,19 @@
-hash: cb430176250bcfe8bafd93aa48d285fa35b08af59b5f9971dcf553a1d6c236ec
-updated: 2017-03-17T16:22:25.024129734-07:00
-imports: []
+hash: b53b5e9a84b9cb3cc4b2d0499e23da2feca1eec318ce9bb717ecf35bf24bf221
+updated: 2017-04-10T13:34:45.671678062-07:00
+imports:
+- name: go.uber.org/atomic
+  version: 3b8db5e93c4c02efbc313e17b2e796b0914a01fb
 testImports:
 - name: github.com/davecgh/go-spew
-  version: 346938d642f2ec3594ed81d874461961cd0faa76
+  version: 6d212800a42e8ab5c146b8ace3490ee17e5225f9
   subpackages:
   - spew
 - name: github.com/pmezard/go-difflib
-  version: 792786c7400a136282c1664665ae0a8db921c6c2
+  version: d8ed2627bdf02c080bf22230dbb337003b7aba2d
   subpackages:
   - difflib
 - name: github.com/stretchr/testify
-  version: 4d4bfba8f1d1027c4fdbe371823030df51419987
+  version: 69483b4bd14f5845b5a1e55bca19e954e827f1d0
   subpackages:
   - assert
+  - require

glide.yaml

@@ -1,5 +1,7 @@
 package: go.uber.org/multierr
-import: []
+import:
+- package: go.uber.org/atomic
+  version: ^1
 testImport:
 - package: github.com/stretchr/testify
   subpackages: