Skip to content

Commit

Permalink
tpl/partials: Fix recently introduced deadlock in partials cache
Browse files Browse the repository at this point in the history
The change in lock logic for `partialCached` in  0927cf7 was naive as it didn't consider cached partials calling other cached partials.

This changeset may look on the large side for this particular issue, but it pulls in part of a working branch, introducing `context.Context` in the template execution.

Note that the context is only partially implemented in this PR, but the upcoming use cases will, as one example, include having access to the top "dot" (e.g. `Page`) all the way down into partials and shortcodes etc.

The earlier benchmarks rerun against master:

```bash
name              old time/op    new time/op    delta
IncludeCached-10    13.6ms ± 2%    13.8ms ± 1%    ~     (p=0.343 n=4+4)

name              old alloc/op   new alloc/op   delta
IncludeCached-10    5.30MB ± 0%    5.35MB ± 0%  +0.96%  (p=0.029 n=4+4)

name              old allocs/op  new allocs/op  delta
IncludeCached-10     74.7k ± 0%     75.3k ± 0%  +0.77%  (p=0.029 n=4+4)
```

Fixes #9519
  • Loading branch information
bep committed Feb 17, 2022
1 parent 667f3a4 commit 9298081
Show file tree
Hide file tree
Showing 9 changed files with 207 additions and 58 deletions.
5 changes: 5 additions & 0 deletions tpl/collections/apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package collections

import (
"context"
"fmt"
"io"
"reflect"
Expand Down Expand Up @@ -51,6 +52,10 @@ func (templateFinder) Execute(t tpl.Template, wr io.Writer, data interface{}) er
return nil
}

func (templateFinder) ExecuteWithContext(ctx context.Context, t tpl.Template, wr io.Writer, data interface{}) error {
return nil
}

func (templateFinder) GetFunc(name string) (reflect.Value, bool) {
if name == "dobedobedo" {
return reflect.Value{}, false
Expand Down
72 changes: 60 additions & 12 deletions tpl/internal/go_templates/texttemplate/hugo_template.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2019 The Hugo Authors. All rights reserved.
// Copyright 2022 The Hugo Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -14,6 +14,7 @@
package template

import (
"context"
"io"
"reflect"

Expand All @@ -39,14 +40,15 @@ type Preparer interface {

// ExecHelper allows some custom eval hooks.
type ExecHelper interface {
GetFunc(tmpl Preparer, name string) (reflect.Value, bool)
GetMethod(tmpl Preparer, receiver reflect.Value, name string) (method reflect.Value, firstArg reflect.Value)
GetMapValue(tmpl Preparer, receiver, key reflect.Value) (reflect.Value, bool)
Init(ctx context.Context, tmpl Preparer)
GetFunc(ctx context.Context, tmpl Preparer, name string) (reflect.Value, reflect.Value, bool)
GetMethod(ctx context.Context, tmpl Preparer, receiver reflect.Value, name string) (method reflect.Value, firstArg reflect.Value)
GetMapValue(ctx context.Context, tmpl Preparer, receiver, key reflect.Value) (reflect.Value, bool)
}

// Executer executes a given template.
type Executer interface {
Execute(p Preparer, wr io.Writer, data interface{}) error
ExecuteWithContext(ctx context.Context, p Preparer, wr io.Writer, data interface{}) error
}

type executer struct {
Expand All @@ -57,27 +59,68 @@ func NewExecuter(helper ExecHelper) Executer {
return &executer{helper: helper}
}

func (t *executer) Execute(p Preparer, wr io.Writer, data interface{}) error {
type (
dataContextKeyType string
hasLockContextKeyType string
)

const (
// The data object passed to Execute or ExecuteWithContext gets stored with this key if not already set.
DataContextKey = dataContextKeyType("data")
// Used in partialCached to signal to nested templates that a lock is already taken.
HasLockContextKey = hasLockContextKeyType("hasLock")
)

// Note: The context is currently not fully implemeted in Hugo. This is a work in progress.
func (t *executer) ExecuteWithContext(ctx context.Context, p Preparer, wr io.Writer, data interface{}) error {
tmpl, err := p.Prepare()
if err != nil {
return err
}

if v := ctx.Value(DataContextKey); v == nil {
ctx = context.WithValue(ctx, DataContextKey, data)
}

value, ok := data.(reflect.Value)
if !ok {
value = reflect.ValueOf(data)
}

state := &state{
ctx: ctx,
helper: t.helper,
prep: p,
tmpl: tmpl,
wr: wr,
vars: []variable{{"$", value}},
}

t.helper.Init(ctx, p)

return tmpl.executeWithState(state, value)
}

func (t *executer) Execute(p Preparer, wr io.Writer, data interface{}) error {
tmpl, err := p.Prepare()
if err != nil {
return err
}

value, ok := data.(reflect.Value)
if !ok {
value = reflect.ValueOf(data)
}

state := &state{
helper: t.helper,
prep: p,
tmpl: tmpl,
wr: wr,
vars: []variable{{"$", value}},
}

return tmpl.executeWithState(state, value)
}

// Prepare returns a template ready for execution.
Expand All @@ -101,8 +144,9 @@ func (t *Template) executeWithState(state *state, value reflect.Value) (err erro
// can execute in parallel.
type state struct {
tmpl *Template
prep Preparer // Added for Hugo.
helper ExecHelper // Added for Hugo.
ctx context.Context // Added for Hugo. The orignal data context.
prep Preparer // Added for Hugo.
helper ExecHelper // Added for Hugo.
wr io.Writer
node parse.Node // current node, for errors
vars []variable // push-down stack of variable values.
Expand All @@ -114,10 +158,11 @@ func (s *state) evalFunction(dot reflect.Value, node *parse.IdentifierNode, cmd
name := node.Ident

var function reflect.Value
// Added for Hugo.
var first reflect.Value
var ok bool
if s.helper != nil {
// Added for Hugo.
function, ok = s.helper.GetFunc(s.prep, name)
function, first, ok = s.helper.GetFunc(s.ctx, s.prep, name)
}

if !ok {
Expand All @@ -127,6 +172,9 @@ func (s *state) evalFunction(dot reflect.Value, node *parse.IdentifierNode, cmd
if !ok {
s.errorf("%q is not a defined function", name)
}
if first != zero {
return s.evalCall(dot, function, cmd, name, args, final, first)
}
return s.evalCall(dot, function, cmd, name, args, final)
}

Expand Down Expand Up @@ -159,7 +207,7 @@ func (s *state) evalField(dot reflect.Value, fieldName string, node parse.Node,
var first reflect.Value
var method reflect.Value
if s.helper != nil {
method, first = s.helper.GetMethod(s.prep, ptr, fieldName)
method, first = s.helper.GetMethod(s.ctx, s.prep, ptr, fieldName)
} else {
method = ptr.MethodByName(fieldName)
}
Expand Down Expand Up @@ -198,7 +246,7 @@ func (s *state) evalField(dot reflect.Value, fieldName string, node parse.Node,
var result reflect.Value
if s.helper != nil {
// Added for Hugo.
result, _ = s.helper.GetMapValue(s.prep, receiver, nameVal)
result, _ = s.helper.GetMapValue(s.ctx, s.prep, receiver, nameVal)
} else {
result = receiver.MapIndex(nameVal)
}
Expand Down
20 changes: 11 additions & 9 deletions tpl/internal/go_templates/texttemplate/hugo_template_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2019 The Hugo Authors. All rights reserved.
// Copyright 2022 The Hugo Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -15,6 +15,7 @@ package template

import (
"bytes"
"context"
"reflect"
"strings"
"testing"
Expand All @@ -35,24 +36,26 @@ func (t TestStruct) Hello2(arg1, arg2 string) string {
return arg1 + " " + arg2
}

type execHelper struct {
type execHelper struct{}

func (e *execHelper) Init(ctx context.Context, tmpl Preparer) {
}

func (e *execHelper) GetFunc(tmpl Preparer, name string) (reflect.Value, bool) {
func (e *execHelper) GetFunc(ctx context.Context, tmpl Preparer, name string) (reflect.Value, reflect.Value, bool) {
if name == "print" {
return zero, false
return zero, zero, false
}
return reflect.ValueOf(func(s string) string {
return "hello " + s
}), true
}), zero, true
}

func (e *execHelper) GetMapValue(tmpl Preparer, m, key reflect.Value) (reflect.Value, bool) {
func (e *execHelper) GetMapValue(ctx context.Context, tmpl Preparer, m, key reflect.Value) (reflect.Value, bool) {
key = reflect.ValueOf(strings.ToLower(key.String()))
return m.MapIndex(key), true
}

func (e *execHelper) GetMethod(tmpl Preparer, receiver reflect.Value, name string) (method reflect.Value, firstArg reflect.Value) {
func (e *execHelper) GetMethod(ctx context.Context, tmpl Preparer, receiver reflect.Value, name string) (method reflect.Value, firstArg reflect.Value) {
if name != "Hello1" {
return zero, zero
}
Expand All @@ -78,12 +81,11 @@ Method: {{ .Hello1 "v1" }}
var b bytes.Buffer
data := TestStruct{S: "sv", M: map[string]string{"a": "av"}}

c.Assert(ex.Execute(templ, &b, data), qt.IsNil)
c.Assert(ex.ExecuteWithContext(context.Background(), templ, &b, data), qt.IsNil)
got := b.String()

c.Assert(got, qt.Contains, "foo")
c.Assert(got, qt.Contains, "hello hugo")
c.Assert(got, qt.Contains, "Map: av")
c.Assert(got, qt.Contains, "Method: v2 v1")

}
28 changes: 28 additions & 0 deletions tpl/partials/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,34 @@ partialCached: foo
`)
}

// Issue 9519
func TestIncludeCachedRecursion(t *testing.T) {
t.Parallel()

files := `
-- config.toml --
baseURL = 'http://example.com/'
-- layouts/index.html --
{{ partials.IncludeCached "p1.html" . }}
-- layouts/partials/p1.html --
{{ partials.IncludeCached "p2.html" . }}
-- layouts/partials/p2.html --
P2
`

b := hugolib.NewIntegrationTestBuilder(
hugolib.IntegrationTestConfig{
T: t,
TxtarString: files,
},
).Build()

b.AssertFileContent("public/index.html", `
P2
`)
}

func TestIncludeCacheHints(t *testing.T) {
t.Parallel()

Expand Down
Loading

0 comments on commit 9298081

Please sign in to comment.