html/template: Execute is not concurrent safe #51344
Comments
mengzhuo
added
the
NeedsInvestigation
|
A much more easier testcase: type Data struct {
counter uint32
}
func (d *Data) Incr() uint32 {
atomic.AddUint32(&d.counter, 1)
return atomic.LoadUint32(&d.counter)
}
func main() {
templ, err := template.New("master").Parse(`
{{ .Incr }}
{{ . }}
`)
if err != nil {
log.Fatal(err)
}
data := &Data{}
go func() {
if err := templ.Execute(ioutil.Discard, data); err != nil {
log.Fatal(err)
}
}()
if err := templ.Execute(ioutil.Discard, data); err != nil {
log.Fatal(err)
}
}I think the doc should changed into
|
mengzhuo
changed the title
|
Just to be clear: This is the current documentation of https://github.com/golang/go/blob/master/src/html/template/template.go#L118 // A template may be executed safely in parallel, although if parallel
// executions share a Writer the output may be interleaved. |
|
The Execute function walk through the data by reflecting fields, for objects that share pointer WILL inevitable be data race . |
|
This may be working as intended: |
@mengzhuo I get a mild heart attack just by reading suggestions like the above.
@seankhliao So, the example given above is obviously simplified and synthetic. The panic comes from tests that fails like 1/100 in a CI environment that I have not managed to pinpoint exactly, but the stack trace is similar. This is the object: The reads you talk about comes from |
|
What you're doing is essentially this, just via the template package. I don't think it's expected or reasonable for library functions to magically synchronize your code. The fact that it's unexported doesn't matter, you've asked for it to be read without synchronization. |
I would say that package main
import (
"fmt"
"io"
"sync"
"sync/atomic"
)
type Data struct {
name string
counter uint32
}
func (d *Data) String() string {
return d.name
}
type Incrementer interface {
Incr() uint32
}
func (d *Data) Incr() uint32 {
atomic.AddUint32(&d.counter, 1)
return atomic.LoadUint32(&d.counter)
}
func main() {
var d Incrementer = &Data{}
var wg sync.WaitGroup
for i := 0; i < 100; i++ {
wg.Add(1)
go func() {
defer wg.Done()
d.Incr()
if stringer, ok := d.(fmt.Stringer); ok {
fmt.Fprintln(io.Discard, stringer.String())
}
}()
}
wg.Wait()
}Which is does not fail. Note that I'm not suggesting the above as a fix, just as an illustration.
A library function's role is to make life easier, and this is especially important in situations where the user lack other synchronization primitives (Go templates) and also a different security setup (dynamic web templates, possibly user provided). But this is not a big thing for me as long as you're not changing the docs to "A template is not concurrent safe to execute". |
@mengzhuo This race is about an un-exported field; you're talking about reflection as something magical, but you could easily argue that the reflect package should be adjusted to skip these fields as they're not relevant for this task (printing the struct). |
|
@bep The package main
import "fmt"
type Data struct {
counter uint32
}
func main() {
fmt.Println(Data{123}) // {123}
} |
|
Like others, I'm not seeing a bug. I don't understand who this could possibly not be a race condition. No matter what, it has to fetch the value of The race is easy enough to avoid with code like package main
import (
"fmt"
"html/template"
"io"
"log"
"sync"
"sync/atomic"
"time"
)
type Data struct {
counter uint32
}
func (d *Data) Incr() uint32 {
atomic.AddUint32(&d.counter, 1)
return atomic.LoadUint32(&d.counter)
}
func (d *Data) String() string {
return fmt.Sprintf("{%d}", atomic.LoadUint32(&d.counter))
}
func main() {
templ, err := template.New("master").Parse(`
{{ .Incr }}
{{ .String }}
`)
if err != nil {
log.Fatal(err)
}
var wg sync.WaitGroup
data := &Data{}
for i := 0; i < 10; i++ {
wg.Add(1)
go func() {
defer wg.Done()
for i := 0; i < 10; i++ {
if err := templ.Execute(io.Discard, data); err != nil {
log.Fatal(err)
}
time.Sleep(23 * time.Millisecond)
}
}()
}
wg.Wait()
log.Println("Done ...")
} |
ianlancetaylor
added
the
WaitingForInfo
|
Timed out in state WaitingForInfo. Closing. (I am just a bot, though. Please speak up if this is a mistake or you have the requested information.) |
Note that I suspect this is the same issue as I reported long time ago in #30286.
What version of Go are you using (
go version)?But I have also experienced this with most earlier Go versions.
Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
Run the following program with the
-raceflag:What did you expect to see?
What did you see instead?
The text was updated successfully, but these errors were encountered: