code review 5450056: html/template: make execution thread-safe (issue 5450056)

[r...@golang.org]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

Description:
html/template: make execution thread-safe
The problem is that execution can modify the template, so it needs
interlocking to have the same thread-safe guarantee as text/template.
Fixes issue 2439.

Please review this at http://codereview.appspot.com/5450056/

Affected files:
M src/pkg/html/template/escape.go
M src/pkg/html/template/template.go

Index: src/pkg/html/template/escape.go
===================================================================
--- a/src/pkg/html/template/escape.go
+++ b/src/pkg/html/template/escape.go
@@ -32,7 +32,7 @@
if err != nil {
// Prevent execution of unsafe templates.
for _, name := range names {
- if t := tmpl.Lookup(name); t != nil {
+ if t := tmpl.set[name]; t != nil {
t.text.Tree = nil
}
}
@@ -520,7 +520,7 @@
if !ok && c1.state != stateError {
return context{
state: stateError,
- // TODO: Find the first node with a line in t.Tree.Root
+ // TODO: Find the first node with a line in t.text.Tree.Root
err: errorf(ErrOutputContext, 0, "cannot compute output context for
template %s", t.Name()),
}
}
Index: src/pkg/html/template/template.go
===================================================================
--- a/src/pkg/html/template/template.go
+++ b/src/pkg/html/template/template.go
@@ -9,6 +9,7 @@
"io"
"io/ioutil"
"path/filepath"
+ "sync"
"text/template"
)

@@ -19,9 +20,14 @@
// We could embed the text/template field, but it's safer not to because
// we need to keep our version of the name space and the underlying
// template's in sync.
- text *template.Template
- // Templates are grouped by sharing the set, a pointer.
- set *map[string]*Template
+ text *template.Template
+ *nameSpace // common to all associated templates
+}
+
+// nameSpace is the data structure shared by all templates in an
association.
+type nameSpace struct {
+ mu sync.Mutex
+ set map[string]*Template
}

// ExecuteTemplate applies the template associated with t that has the
given name
@@ -31,8 +37,10 @@
if tmpl == nil {
return fmt.Errorf("template: no template %q associated with
template %q", name, t.Name())
}
+ t.nameSpace.mu.Lock()
+ defer t.nameSpace.mu.Unlock()
if !tmpl.escaped {
- if err := escapeTemplates(tmpl, name); err != nil { // TODO: make a
method of set?
+ if err := escapeTemplates(tmpl, name); err != nil {
return err
}
}
@@ -44,7 +52,9 @@
// to the set. If a template is redefined, the element in the set is
// overwritten with the new definition.
func (t *Template) Parse(src string) (*Template, error) {
+ t.nameSpace.mu.Lock()
t.escaped = false
+ t.nameSpace.mu.Unlock()
ret, err := t.text.Parse(src)
if err != nil {
return nil, err
@@ -52,11 +62,13 @@
// In general, all the named templates might have changed underfoot.
// Regardless, some new ones may have been defined.
// The template.Template set has been updated; update ours.
+ t.nameSpace.mu.Lock()
+ defer t.nameSpace.mu.Unlock()
for _, v := range ret.Templates() {
name := v.Name()
- tmpl := t.Lookup(name)
+ tmpl := t.set[name]
if tmpl == nil {
- tmpl = t.New(name)
+ tmpl = t.new(name)
}
tmpl.escaped = false
tmpl.text = v
@@ -67,6 +79,8 @@
// Execute applies a parsed template to the specified data object,
// writing the output to wr.
func (t *Template) Execute(wr io.Writer, data interface{}) error {
+ t.nameSpace.mu.Lock()
+ defer t.nameSpace.mu.Unlock()
if !t.escaped {
if err := escapeTemplates(t, t.Name()); err != nil {
return err
@@ -88,13 +102,14 @@

// New allocates a new HTML template with the given name.
func New(name string) *Template {
- set := make(map[string]*Template)
tmpl := &Template{
false,
template.New(name),
- &set,
+ &nameSpace{
+ set: make(map[string]*Template),
+ },
}
- (*tmpl.set)[name] = tmpl
+ tmpl.set[name] = tmpl
return tmpl
}

@@ -102,12 +117,19 @@
// and with the same delimiters. The association, which is transitive,
// allows one template to invoke another with a {{template}} action.
func (t *Template) New(name string) *Template {
+ t.nameSpace.mu.Lock()
+ defer t.nameSpace.mu.Unlock()
+ return t.new(name)
+}
+
+// new is the implementation of New, without the lock.
+func (t *Template) new(name string) *Template {
tmpl := &Template{
false,
t.text.New(name),
- t.set,
+ t.nameSpace,
}
- (*tmpl.set)[name] = tmpl
+ tmpl.set[name] = tmpl
return tmpl
}

@@ -138,7 +160,9 @@
// Lookup returns the template with the given name that is associated with
t,
// or nil if there is no such template.
func (t *Template) Lookup(name string) *Template {
- return (*t.set)[name]
+ t.nameSpace.mu.Lock()
+ defer t.nameSpace.mu.Unlock()
+ return t.set[name]
}

// Must panics if err is non-nil in the same way as template.Must.

[a...@golang.org]

http://codereview.appspot.com/5450056/diff/1/src/pkg/html/template/template.go
File src/pkg/html/template/template.go (right):

http://codereview.appspot.com/5450056/diff/1/src/pkg/html/template/template.go#newcode41
src/pkg/html/template/template.go:41: defer t.nameSpace.mu.Unlock()
Wouldn't it be better to release this mutex after escapeTemplates has
executed? There's no reason to lock while text.ExecuteTemplate is
running, is there?

http://codereview.appspot.com/5450056/diff/1/src/pkg/html/template/template.go#newcode83
src/pkg/html/template/template.go:83: defer t.nameSpace.mu.Unlock()
ditto

http://codereview.appspot.com/5450056/

[r...@golang.org]

Hello golan...@googlegroups.com, a...@golang.org (cc:
golan...@googlegroups.com),

Please take another look.

http://codereview.appspot.com/5450056/

[a...@golang.org]

LGTM

http://codereview.appspot.com/5450056/

[r...@golang.org]

*** Submitted as
http://code.google.com/p/go/source/detail?r=e22dbef7457e ***

html/template: make execution thread-safe
The problem is that execution can modify the template, so it needs
interlocking to have the same thread-safe guarantee as text/template.
Fixes issue 2439.

R=golang-dev, adg
CC=golang-dev
http://codereview.appspot.com/5450056

http://codereview.appspot.com/5450056/