Fix data race and optimize performance (nicksnyder#183)

Author: nicksnyder

.travis.yml

@@ -3,15 +3,15 @@ language: go
 matrix:
   include:
     - go: 1.9.x
-      env: PKG='./...'
+      env: GO111MODULE=on
     - go: 1.12.x
-      env: PKG='./...' COVER='-coverprofile=coverage.txt -covermode=count'
+      env: GO111MODULE=on COVER='-coverprofile=coverage.txt -covermode=atomic'
 
 install:
-  - go get -t -v $PKG
+  - go get -t -v ./...
 
 script:
-  - go test $COVER $PKG
+  - go test -race $COVER ./...
 
 after_success:
   - bash <(curl -s https://codecov.io/bash)

i18n/bundle_test.go

@@ -1,6 +1,7 @@
 package i18n
 
 import (
+	"fmt"
 	"reflect"
 	"testing"
 
@@ -31,6 +32,34 @@ var everythingMessage = MustNewMessage(map[string]string{
 	"other":       "other translation",
 })
 
+func TestConcurrentAccess(t *testing.T) {
+	bundle := NewBundle(language.English)
+	bundle.RegisterUnmarshalFunc("toml", toml.Unmarshal)
+	bundle.MustParseMessageFileBytes([]byte(`
+# Comment
+hello = "world"
+`), "en.toml")
+
+	count := 10
+	errch := make(chan error, count)
+	for i := 0; i < count; i++ {
+		go func() {
+			localized := NewLocalizer(bundle, "en").MustLocalize(&LocalizeConfig{MessageID: "hello"})
+			if localized != "world" {
+				errch <- fmt.Errorf(`expected "world"; got %q`, localized)
+			} else {
+				errch <- nil
+			}
+		}()
+	}
+
+	for i := 0; i < count; i++ {
+		if err := <-errch; err != nil {
+			t.Fatal(err)
+		}
+	}
+}
+
 func TestPseudoLanguage(t *testing.T) {
 	bundle := NewBundle(language.English)
 	bundle.RegisterUnmarshalFunc("toml", toml.Unmarshal)

i18n/message_template.go

@@ -1,7 +1,6 @@
 package i18n
 
 import (
-	"bytes"
 	"fmt"
 
 	"text/template"
@@ -19,12 +18,12 @@ type MessageTemplate struct {
 // NewMessageTemplate returns a new message template.
 func NewMessageTemplate(m *Message) *MessageTemplate {
 	pluralTemplates := map[plural.Form]*internal.Template{}
-	setPluralTemplate(pluralTemplates, plural.Zero, m.Zero)
-	setPluralTemplate(pluralTemplates, plural.One, m.One)
-	setPluralTemplate(pluralTemplates, plural.Two, m.Two)
-	setPluralTemplate(pluralTemplates, plural.Few, m.Few)
-	setPluralTemplate(pluralTemplates, plural.Many, m.Many)
-	setPluralTemplate(pluralTemplates, plural.Other, m.Other)
+	setPluralTemplate(pluralTemplates, plural.Zero, m.Zero, m.LeftDelim, m.RightDelim)
+	setPluralTemplate(pluralTemplates, plural.One, m.One, m.LeftDelim, m.RightDelim)
+	setPluralTemplate(pluralTemplates, plural.Two, m.Two, m.LeftDelim, m.RightDelim)
+	setPluralTemplate(pluralTemplates, plural.Few, m.Few, m.LeftDelim, m.RightDelim)
+	setPluralTemplate(pluralTemplates, plural.Many, m.Many, m.LeftDelim, m.RightDelim)
+	setPluralTemplate(pluralTemplates, plural.Other, m.Other, m.LeftDelim, m.RightDelim)
 	if len(pluralTemplates) == 0 {
 		return nil
 	}
@@ -34,9 +33,13 @@ func NewMessageTemplate(m *Message) *MessageTemplate {
 	}
 }
 
-func setPluralTemplate(pluralTemplates map[plural.Form]*internal.Template, pluralForm plural.Form, src string) {
+func setPluralTemplate(pluralTemplates map[plural.Form]*internal.Template, pluralForm plural.Form, src, leftDelim, rightDelim string) {
 	if src != "" {
-		pluralTemplates[pluralForm] = &internal.Template{Src: src}
+		pluralTemplates[pluralForm] = &internal.Template{
+			Src:        src,
+			LeftDelim:  leftDelim,
+			RightDelim: rightDelim,
+		}
 	}
 }
 
@@ -58,15 +61,5 @@ func (mt *MessageTemplate) Execute(pluralForm plural.Form, data interface{}, fun
 			messageID:  mt.Message.ID,
 		}
 	}
-	if err := t.Parse(mt.LeftDelim, mt.RightDelim, funcs); err != nil {
-		return "", err
-	}
-	if t.Template == nil {
-		return t.Src, nil
-	}
-	var buf bytes.Buffer
-	if err := t.Template.Execute(&buf, data); err != nil {
-		return "", err
-	}
-	return buf.String(), nil
+	return t.Execute(funcs, data)
 }

internal/template.go

@@ -1,26 +1,51 @@
 package internal
 
 import (
+	"bytes"
 	"strings"
+	"sync"
 	gotemplate "text/template"
 )
 
 // Template stores the template for a string.
 type Template struct {
-	Src      string
-	Template *gotemplate.Template
-	ParseErr *error
+	Src        string
+	LeftDelim  string
+	RightDelim string
+
+	parseOnce      sync.Once
+	parsedTemplate *gotemplate.Template
+	parseError     error
 }
 
-func (t *Template) Parse(leftDelim, rightDelim string, funcs gotemplate.FuncMap) error {
-	if t.ParseErr == nil {
-		if strings.Contains(t.Src, leftDelim) {
-			gt, err := gotemplate.New("").Funcs(funcs).Delims(leftDelim, rightDelim).Parse(t.Src)
-			t.Template = gt
-			t.ParseErr = &err
-		} else {
-			t.ParseErr = new(error)
-		}
+func (t *Template) Execute(funcs gotemplate.FuncMap, data interface{}) (string, error) {
+	leftDelim := t.LeftDelim
+	if leftDelim == "" {
+		leftDelim = "{{"
+	}
+	if !strings.Contains(t.Src, leftDelim) {
+		// Fast path to avoid parsing a template that has no actions.
+		return t.Src, nil
+	}
+
+	var gt *gotemplate.Template
+	var err error
+	if funcs == nil {
+		t.parseOnce.Do(func() {
+			// If funcs is nil, then we only need to parse this template once.
+			t.parsedTemplate, t.parseError = gotemplate.New("").Delims(t.LeftDelim, t.RightDelim).Parse(t.Src)
+		})
+		gt, err = t.parsedTemplate, t.parseError
+	} else {
+		gt, err = gotemplate.New("").Delims(t.LeftDelim, t.RightDelim).Funcs(funcs).Parse(t.Src)
+	}
+
+	if err != nil {
+		return "", err
+	}
+	var buf bytes.Buffer
+	if err := gt.Execute(&buf, data); err != nil {
+		return "", err
 	}
-	return *t.ParseErr
+	return buf.String(), nil
 }

internal/template_test.go

@@ -1,54 +1,77 @@
 package internal
 
 import (
-	"bytes"
-	"fmt"
 	"testing"
 	"text/template"
 )
 
-func TestParse(t *testing.T) {
-	tmpl := &Template{Src: "hello"}
-	if err := tmpl.Parse("", "", nil); err != nil {
-		t.Fatal(err)
-	}
-	if tmpl.ParseErr == nil {
-		t.Fatal("expected non-nil parse error")
-	}
-	if tmpl.Template == nil {
-		t.Fatal("expected non-nil template")
+func TestExecute(t *testing.T) {
+	tests := []struct {
+		template *Template
+		funcs    template.FuncMap
+		data     interface{}
+		result   string
+		err      string
+		noallocs bool
+	}{
+		{
+			template: &Template{
+				Src: "hello",
+			},
+			result:   "hello",
+			noallocs: true,
+		},
+		{
+			template: &Template{
+				Src: "hello {{.Noun}}",
+			},
+			data: map[string]string{
+				"Noun": "world",
+			},
+			result: "hello world",
+		},
+		{
+			template: &Template{
+				Src: "hello {{world}}",
+			},
+			funcs: template.FuncMap{
+				"world": func() string {
+					return "world"
+				},
+			},
+			result: "hello world",
+		},
+		{
+			template: &Template{
+				Src: "hello {{",
+			},
+			err:      "template: :1: unexpected unclosed action in command",
+			noallocs: true,
+		},
 	}
-}
 
-func TestParseError(t *testing.T) {
-	expectedErr := fmt.Errorf("expected error")
-	tmpl := &Template{ParseErr: &expectedErr}
-	if err := tmpl.Parse("", "", nil); err != expectedErr {
-		t.Fatalf("expected %#v; got %#v", expectedErr, err)
+	for _, test := range tests {
+		t.Run(test.template.Src, func(t *testing.T) {
+			result, err := test.template.Execute(test.funcs, test.data)
+			if actual := str(err); actual != test.err {
+				t.Errorf("expected err %q; got %q", test.err, actual)
+			}
+			if result != test.result {
+				t.Errorf("expected result %q; got %q", test.result, result)
+			}
+			allocs := testing.AllocsPerRun(10, func() {
+				_, _ = test.template.Execute(test.funcs, test.data)
+			})
+			if test.noallocs && allocs > 0 {
+				t.Errorf("expected no allocations; got %f", allocs)
+			}
+		})
 	}
 }
 
-func TestParseWithFunc(t *testing.T) {
-	tmpl := &Template{Src: "{{foo}}"}
-	funcs := template.FuncMap{
-		"foo": func() string {
-			return "bar"
-		},
-	}
-	if err := tmpl.Parse("", "", funcs); err != nil {
-		t.Fatal(err)
-	}
-	if tmpl.ParseErr == nil {
-		t.Fatal("expected non-nil parse error")
-	}
-	if tmpl.Template == nil {
-		t.Fatal("expected non-nil template")
-	}
-	var buf bytes.Buffer
-	if tmpl.Template.Execute(&buf, nil) != nil {
-		t.Fatal("expected nil template execute error")
-	}
-	if buf.String() != "bar" {
-		t.Fatalf("expected bar; got %s", buf.String())
+func str(err error) string {
+	if err == nil {
+		return ""
 	}
+	return err.Error()
 }