html/template: fix Clone so that t.Lookup(t.Name()) yields t

Template.escape makes the assumption that t.Lookup(t.Name()) is t (escapeTemplate looks up the associated template by name and sets escapeErr appropriately). This assumption did not hold for a Cloned template, because the template associated with t.Name() was a second copy of the original. Add a test for the assumption that t.Lookup(t.Name()) == t. One effect of this broken assumption was #16101: parallel Executes racily accessed the template namespace because each Execute call saw t.escapeErr == nil and re-escaped the template concurrently with read accesses occurring outside the namespace mutex. Add a test for this race. Related to #12996 and CL 16104. Fixes #16101 Change-Id: I59831d0847abbabb4ef9135f2912c6ce982f9837 Reviewed-on: https://go-review.googlesource.com/31092 Run-TryBot: Rob Pike <r@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rob Pike <r@golang.org>

html/template: fix Clone so that t.Lookup(t.Name()) yields t
Template.escape makes the assumption that t.Lookup(t.Name()) is t (escapeTemplate looks up the associated template by name and sets escapeErr appropriately). This assumption did not hold for a Cloned template, because the template associated with t.Name() was a second copy of the original. Add a test for the assumption that t.Lookup(t.Name()) == t. One effect of this broken assumption was #16101: parallel Executes racily accessed the template namespace because each Execute call saw t.escapeErr == nil and re-escaped the template concurrently with read accesses occurring outside the namespace mutex. Add a test for this race. Related to #12996 and CL 16104. Fixes #16101 Change-Id: I59831d0847abbabb4ef9135f2912c6ce982f9837 Reviewed-on: https://go-review.googlesource.com/31092 Run-TryBot: Rob Pike <r@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rob Pike <r@golang.org>
cd2c9df7 · Caleb Spare · Rob Pike · d8cbc2c9 · cd2c9df7 · cd2c9df7
Commit cd2c9df7 authored Oct 14, 2016 by Caleb Spare Committed by Rob Pike Oct 17, 2016
Hide whitespace changes
Inline Side-by-side

Showing with 38 additions and 0 deletions

src/html/template/clone_test.go src/html/template/clone_test.go +35 -0

src/html/template/template.go src/html/template/template.go +3 -0

No files found.
--- a/src/html/template/clone_test.go
+++ b/src/html/template/clone_test.go
@@ -8,6 +8,7 @@ import (
 	"bytes"
 	"errors"
 	"io/ioutil"
+	"sync"
 	"testing"
 	"text/template/parse"
 )
@@ -194,3 +195,37 @@ func TestFuncMapWorksAfterClone(t *testing.T) {
 		t.Errorf("clone error message mismatch want %q got %q", wantErr, gotErr)
 	}
 }
+// https://golang.org/issue/16101
+func TestTemplateCloneExecuteRace(t *testing.T) {
+	const (
+		input   = `<title>{{block "a" .}}a{{end}}</title><body>{{block "b" .}}b{{end}}<body>`
+		overlay = `{{define "b"}}A{{end}}`
+	)
+	outer := Must(New("outer").Parse(input))
+	tmpl := Must(Must(outer.Clone()).Parse(overlay))
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for i := 0; i < 100; i++ {
+				if err := tmpl.Execute(ioutil.Discard, "data"); err != nil {
+					panic(err)
+				}
+			}
+		}()
+	}
+	wg.Wait()
+}
+func TestTemplateCloneLookup(t *testing.T) {
+	// Template.escape makes an assumption that the template associated
+	// with t.Name() is t. Check that this holds.
+	tmpl := Must(New("x").Parse("a"))
+	tmpl = Must(tmpl.Clone())
+	if tmpl.Lookup(tmpl.Name()) != tmpl {
+		t.Error("after Clone, tmpl.Lookup(tmpl.Name()) != tmpl")
+	}
+}
--- a/src/html/template/template.go
+++ b/src/html/template/template.go
@@ -240,6 +240,9 @@ func (t *Template) Clone() (*Template, error) {
 	ret.set[ret.Name()] = ret
 	for _, x := range textClone.Templates() {
 		name := x.Name()
+		if name == ret.Name() {
+			continue
+		}
 		src := t.set[name]
 		if src == nil || src.escapeErr != nil {
 			return nil, fmt.Errorf("html/template: cannot Clone %q after it has executed", t.Name())