Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

html/template: concurrent map read and map write in execute #16101

Closed
bep opened this issue Jun 17, 2016 · 20 comments
Closed

html/template: concurrent map read and map write in execute #16101

bep opened this issue Jun 17, 2016 · 20 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@bep
Copy link
Contributor

bep commented Jun 17, 2016

Go 1.6.2.

See gohugoio/hugo#2224

In an ideal world, I would have investigated further before posting this issue, but I'm short of time these days -- thought it would be better to just post it than to wait for an open time window.

I suspect this may be a artifact of the new block keyword in Go .1.6, which changed the semantics of when a template could be changed.

@adg adg self-assigned this Jun 17, 2016
@adg adg added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 17, 2016
@adg adg added this to the Go1.7 milestone Jun 17, 2016
@christophermancini
Copy link

To add some more context. I have a website that I built using Hugo. It has over 4500 pages. I started implementing the block template functionality and it appears to operate as expected when using it with standard content templates. Whenever I attempt to use it with taxonomies, it crashes almost every time with the error attached to the Hugo issue.

Unfortunately, I cannot make the repo for the site open to the public due to legal limitations. But I will be glad to assist with the investigation however I can.

@adg adg modified the milestones: Go1.8, Go1.7 Jun 19, 2016
@adg
Copy link
Contributor

adg commented Jun 19, 2016

What do you mean by "taxonomies"?

Without a small example that we can use to reproduce the issue, it's going to be hard to diagnose the problem.

We're not going to be able to tackle this for the 1.7 release, as we are now too close to a release candidate. But since this was not a regression between 1.6 and 1.7, it's okay to defer it for 1.8.

@davecheney
Copy link
Contributor

Can you please build a version of hugo using the race runtime and post the
race report. That should be sufficient to isolate the offending code.

On Sat, Jun 18, 2016 at 9:20 PM, Christopher Mancini <
notifications@github.com> wrote:

To add some more context. I have a website that I built using Hugo. It has
over 4500 pages. I started implementing the block template functionality
and it appears to operate as expected when using it with standard content
templates. Whenever I attempt to use it with taxonomies, it crashes almost
every time with the error attached to the Hugo issue.

Unfortunately, I cannot make the repo for the site open to the public due
to legal limitations. But I will be glad to assist with the investigation
however I can.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#16101 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAAcA_tvhxTTivkrDMtrjk-oYdVequSMks5qM9SGgaJpZM4I4zSM
.

@christophermancini
Copy link

@adg @davecheney somehow the issue no longer occurred after I finished implementing Go template blocks across the entire site. No idea why I can't reproduce it anymore.
gohugoio/hugo#2224 (comment)

@adg
Copy link
Contributor

adg commented Jun 19, 2016

OK, thanks for letting us know.

@adg adg closed this as completed Jun 19, 2016
@bep
Copy link
Contributor Author

bep commented Jul 8, 2016

@adg I opened this issue, and this is still an open issue in Hugo, and most signals point to a real issue in Go. That it is somehow "no longer an issue" for the original reporter does not equal "no longer an issue" (he moved his templates around, aka worked his way around the issue). There is a stack trace.

@adg
Copy link
Contributor

adg commented Jul 8, 2016

@bep if there's an issue to be fixed in the template package, then we need to track it down. To do that we need a self-contained example that demonstrates the problem.

@bep
Copy link
Contributor Author

bep commented Oct 11, 2016

We have a reproduce-able case in a private Hugo site repo, I will try to boil it down, but haven't had any success yet.

But here is the trace from the race detector:

WARNING: DATA RACE
Read at 0x00c424139110 by goroutine 6:
  runtime.mapaccess1_faststr()
      /usr/local/go/src/runtime/hashmap_fast.go:192 +0x0
  text/template.(*state).walkTemplate()
      /usr/local/go/src/text/template/exec.go:369 +0x12b
  text/template.(*state).walk()
      /usr/local/go/src/text/template/exec.go:239 +0x3a4
  text/template.(*state).walk()
      /usr/local/go/src/text/template/exec.go:234 +0x1f7
  text/template.(*Template).execute()
      /usr/local/go/src/text/template/exec.go:189 +0x4a2
  text/template.(*Template).Execute()
      /usr/local/go/src/text/template/exec.go:175 +0x64
  html/template.(*Template).Execute()
      /usr/local/go/src/html/template/template.go:104 +0xd0
  github.com/spf13/hugo/hugolib.(*Site).renderThing()
      /Users/bep/go/src/github.com/spf13/hugo/hugolib/site.go:2436 +0xcf
  github.com/spf13/hugo/hugolib.(*Site).renderForLayouts()
      /Users/bep/go/src/github.com/spf13/hugo/hugolib/site.go:2408 +0x2b8
  github.com/spf13/hugo/hugolib.(*Site).renderAndWritePage()
      /Users/bep/go/src/github.com/spf13/hugo/hugolib/site.go:2316 +0x138
  github.com/spf13/hugo/hugolib.pageRenderer()
      /Users/bep/go/src/github.com/spf13/hugo/hugolib/site.go:1658 +0x23e

Previous write at 0x00c424139110 by goroutine 7:
  runtime.mapassign1()
      /usr/local/go/src/runtime/hashmap.go:442 +0x0
  text/template.(*Template).associate()
      /usr/local/go/src/text/template/template.go:216 +0x166
  text/template.(*Template).AddParseTree()
      /usr/local/go/src/text/template/template.go:128 +0x26c
  html/template.(*escaper).commit()
      /usr/local/go/src/html/template/escape.go:746 +0x30c
  html/template.escapeTemplate()
      /usr/local/go/src/html/template/escape.go:39 +0x4fd
  html/template.(*Template).escape()
      /usr/local/go/src/html/template/template.go:85 +0x2fb
  html/template.(*Template).Execute()
      /usr/local/go/src/html/template/template.go:101 +0x3c
  github.com/spf13/hugo/hugolib.(*Site).renderThing()
      /Users/bep/go/src/github.com/spf13/hugo/hugolib/site.go:2436 +0xcf
  github.com/spf13/hugo/hugolib.(*Site).renderForLayouts()
      /Users/bep/go/src/github.com/spf13/hugo/hugolib/site.go:2408 +0x2b8
  github.com/spf13/hugo/hugolib.(*Site).renderAndWritePage()
      /Users/bep/go/src/github.com/spf13/hugo/hugolib/site.go:2316 +0x138
  github.com/spf13/hugo/hugolib.pageRenderer()
      /Users/bep/go/src/github.com/spf13/hugo/hugolib/site.go:1658 +0x23e

What I see above is:

  • Both stacks on the Go side starts in html/template.Execute
  • Execute do both reads and writes to the internal template map
  • Execute is documented as A template may be executed safely in parallel.

gohugoio/hugo#2549

@rdwatters
Copy link

@bep @adg Suggesting fixes this close to the metal is out of my wheelhouse, but the private repo is mine. If there is value in adding you as a collaborator, @adg, I'm happy to do so. Thanks much.

@bep
Copy link
Contributor Author

bep commented Oct 13, 2016

@adg here is a repro that crashes consistently on my MacBook dual core / macOS Sierra / Go 1.7.1:

package main

import (
    "html/template"
    "io/ioutil"
    "log"
    "sync"
    "time"
)

var (
    overlayTmpl *template.Template
    masterTmpl  *template.Template
)

func main() {
    const (
        master = `
<!DOCTYPE HTML>
<html>
<head>
<title>
{{block "a" . }}Default{{end}}
</title>
</head>
<body>
{{block "b" .}}Default{{end}}
{{block "c" .}}Default{{end}}
</body>
</html>
`
        overlay = `
{{define "b"}}A{{end}} 
`
    )

    var err error

    masterTmpl, err = template.New("master").Parse(master)
    if err != nil {
        log.Fatal(err)
    }
    overlayTmpl, err = template.Must(masterTmpl.Clone()).Parse(overlay)
    if err != nil {
        log.Fatal(err)
    }

    var wg sync.WaitGroup

    for i := 0; i < 100; i++ {
        wg.Add(1)
        go func() {
            defer wg.Done()
            for i := 0; i < 1000; i++ {
                if err := overlayTmpl.Execute(ioutil.Discard, "data"); err != nil {
                    log.Fatal(err)
                }
                time.Sleep(23 * time.Millisecond)
            }
        }()
    }

    wg.Wait()
    log.Println("Done ...")
}
go run main.go -race &> race.txt

This is becoming a fairly painful issue in Hugo -- and any advice for a workaround would be appreciated.

@mrjrieke
Copy link

Nice. @bep
go version go1.7.1 linux/amd64

go run main.go -race
goroutine 100 [semacquire]:
sync.runtime_Semacquire(0xc42000e4d4)
/usr/local/go/src/runtime/sema.go:47 +0x30
sync.(_Mutex).Lock(0xc42000e4d0)
/usr/local/go/src/sync/mutex.go:85 +0xd0
html/template.(_Template).escape(0xc420012db0, 0x0, 0x0)
/usr/local/go/src/html/template/template.go:79 +0x53
html/template.(*Template).Execute(0xc420012db0, 0x602640, 0xc42000e320, 0x526a00, 0xc420e331e0, 0x0, 0x0)
/usr/local/go/src/html/template/template.go:101 +0x2f
main.main.func1(0xc42000e5d0)
/home/jrieke/workspace/Bjorn/src/bjorn/main.go:55 +0xdc
created by main.main
/home/jrieke/workspace/Bjorn/src/bjorn/main.go:60 +0x32a

@rsc rsc assigned quentinmit and unassigned adg Oct 13, 2016
@cespare
Copy link
Contributor

cespare commented Oct 14, 2016

This is an html/template bug, not a text/template bug. Can someone change the title?

The fix is simple, but the test is less so. I'll mail a CL shortly.

@bep
Copy link
Contributor Author

bep commented Oct 14, 2016

If the "fix is simple" I suggest adding this to the 1.7.2 milestone: It renders the block feature pretty much useless in a hard to understand and destructive way.

@gopherbot
Copy link

CL https://golang.org/cl/31092 mentions this issue.

@bradfitz
Copy link
Contributor

@bep, the bar for point release cherry picks is very high. If it's not a regression from Go 1.6 then it probably doesn't qualify.

@cespare
Copy link
Contributor

cespare commented Oct 14, 2016

@bep here's a hacky workaround that should work in the meantime.

    masterTmpl, err = template.New("master").Parse(master)
    if err != nil {
        log.Fatal(err)
    }
    overlayTmpl, err = template.Must(masterTmpl.Clone()).Parse(overlay)
    if err != nil {
        log.Fatal(err)
    }
    overlayTmpl = overlayTmpl.Lookup(overlayTmpl.Name()) // horrible hack

That fixes your repro, at least.

@bep
Copy link
Contributor Author

bep commented Oct 14, 2016

@bradfitz I understand that, but don't blame me for trying :-) ... and I have seen several exceptions to that "regression rule" in previous point releases

@cespare thanks for the workaround tip -- I will test it right away.

@bradfitz
Copy link
Contributor

There are no hard rules. Only a set of considerations which weigh into the decision. I'll let others decide on this one since I'm not the most familiar with this code.

@bep
Copy link
Contributor Author

bep commented Oct 14, 2016

@cespare your suggested workaround works great for Hugo and myself (gohugoio/hugo@5882567). Thanks again.

@quentinmit quentinmit assigned cespare and unassigned quentinmit Oct 14, 2016
@bep bep changed the title text/template: concurrent map read and map write in execute html/template: concurrent map read and map write in execute Oct 14, 2016
@gopherbot
Copy link

CL https://golang.org/cl/33210 mentions this issue.

gopherbot pushed a commit that referenced this issue Nov 15, 2016
This change redoes the fix for #16101 (CL 31092) in a different way by
making t.Clone return the template associated with the t.Name() while
allowing for the case that a template of the same name is define-d.

Fixes #17735.

Change-Id: I1e69672390a4c81aa611046a209008ae4a3bb723
Reviewed-on: https://go-review.googlesource.com/33210
Run-TryBot: Caleb Spare <cespare@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@golang golang locked and limited conversation to collaborators Nov 14, 2017
@rsc rsc unassigned cespare Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

10 participants