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

Data race in Template.Parse #181

Closed
bep opened this issue Jun 2, 2019 · 4 comments · Fixed by #183
Closed

Data race in Template.Parse #181

bep opened this issue Jun 2, 2019 · 4 comments · Fixed by #183

Comments

@bep
Copy link
Contributor

bep commented Jun 2, 2019

I get a data race reported in some of Hugo's tests:

https://travis-ci.org/gohugoio/hugo/jobs/540393974

https://github.com/nicksnyder/go-i18n/blob/master/internal/template.go#L16

@bep
Copy link
Contributor Author

bep commented Jun 3, 2019

I tried to have a go at fixing this, and my first instinct would be to:

t.parseInit.Do(func() {
		if strings.Contains(t.Src, leftDelim) {
			gt, err := gotemplate.New("").Funcs(funcs).Delims(leftDelim, rightDelim).Parse(t.Src)
			t.Template = gt
			t.ParseErr = err
		}
	})

But that doesn't work since you take leftDelim etc. as argument to Parse and hence needs to be reparsed. Locking with a mutex would work, but sounds unneededly ineffecient; why not parse the template once when you create the Template? Do you support multiple delimiters? Possibly related to #182

@nicksnyder
Copy link
Owner

Thanks for reporting, I will take a look!

why not parse the template once when you create the Template?

This might be the right path forward.

@nicksnyder
Copy link
Owner

Do you support multiple delimiters?

Technically, each translation could define its own set of delimiters, but that actually isn't the problem here I think since those delimiters would be stored in the translation file and I could lift those parameter out of the parse function.

why not parse the template once when you create the Template?

The limiting factor here is actually Funcs from the LocalizeConfig. Since we don't know this when the templates are loaded it needs to be lazily parsed. Might need to lock; still thinking about it.

@nicksnyder
Copy link
Owner

This should be fixed in v2.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants