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

A template is not concurrent safe to execute.

CC @robpike @mvdan

Just to be clear: This is the current documentation of Execute:

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 .
Putting a mutex in the template for Execute will solve this issue, unfortunately this will cause a performance regression on objects which not share any pointer.

This may be working as intended:
While you protect writes to *Data.counter by sync/atomic, the reads from template {{ . }} aren't so there's a race.

A template is not concurrent safe to execute.

@mengzhuo I get a mild heart attack just by reading suggestions like the above. html/template.Execute has existed since Go 1.1/1.0 and, a bold statement, is most likely the single method in Go's stdlib that has seen most parallel execution and variation under the race detector. With that I think my original issue title would be more descriptive.

While you protect writes to *Data.counter by sync/atomic, the reads from template {{ . }} aren't so there's a race.

@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:

type Data struct {
	counter uint32
}

The reads you talk about comes from html/template.indirectToStringerOrError. It should certainly be both possible and the expected thing to create a string representation of *Data without a data race related to the unexported field counter.

What you're doing is essentially this, just via the template package.

package main

import (
	"fmt"
	"io"
	"sync"
	"sync/atomic"
)

type Data struct {
	counter uint32
}

func (d *Data) Incr() uint32 {
	atomic.AddUint32(&d.counter, 1)
	return atomic.LoadUint32(&d.counter)
}

func main() {
	d := &Data{}
	var wg sync.WaitGroup
	for i := 0; i < 100; i++ {
		wg.Add(1)
		go func() {
			defer wg.Done()
			d.Incr()
			fmt.Fprintln(io.Discard, d)
		}()
	}
	wg.Wait()
}

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.

What you're doing is essentially this, just via the template package.

I would say that fmt.Fprintln's behaviour is also surprising and buggy, but I would argue that this is similar to:

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.

I don't think it's expected or reasonable for library functions to magically synchronize your code. T

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".

The Execute function walk through the data by reflecting fields, for objects that share pointer WILL inevitable be data race .

@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 fmt package prints unexported fields by default. It's too late to change that now:

package main

import "fmt"

type Data struct {
	counter uint32
}

func main() {
	fmt.Println(Data{123}) // {123}
}

https://go.dev/play/p/HP0mvJiCaWJ

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 data.counter in order to print it. Whether counter is exported or not is not relevant here. What is the argument that there isn't a race condition here?

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 ...")
}

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.)