Ensure page translation CPs are initialized

[ptgott]

PR #9342 introduced a regression in which calling .Translations in a
template and calling RenderString on a translated Page caused a nil
pointer dereference. The issue was that some Pages returned from
.Translations had a nil cp field at the time the calling template was
being executed.

While PR #9342 had attempted to ensure that all ContentProviders were
initialized for translations at build time, it only performed the
initialization for receivers of ContentProvider methods such as
.Summary. However, the ContentProvider's *pageState.pageOutput.cp would
remain uninitialized, causing the nil pointer dereference.

This change edits the .Translations and .AllTranslations methods to
ensure that all of a page's translations have an initialized
content provider in time for a template to be executed.

Since LazyContentProvider is no longer needed with this approach, this
change also reverts the following commits:

• cdcd15b
• 25d645f

Fixes #9383

[bep]

Thanks for this.

But I don't think we can solve this like this. Most notably is the data race which, if it's not already failing the build, it will if you move the .Translations into the single template.

I suspect that the cleanest, working solution we will find without doing too much work is to expand on the patch we added a few days ago: Make sure that the lazy content provider is triggered on all access to cp (including RenderString).

[ptgott]

Thanks for the feedback. Would you mind explaining the data race a bit more? Which goroutines, processes, I/O, etc. would be racing?

I'll spend some time early this week implementing the approach you've suggested.

[bep]

1. We render pages in parallel.
2. Given the below, AllTranslations may be called from multiple go routines on the same page.
3. Any of the code lines reading or writing mutable page state below will be a potential data race (e.g. if tp.pageOutput.cp == nil and tp.initContentProvider(cp)). Go's race detector will fail the tests when run with go test -race (which we do on the CI server), but I guess you need to have test cases that trigger it.

func (p *pageState) AllTranslations() page.Pages {
	p.s.h.init.translations.Do()

	// Hugo attempts to reuse content providers while preparing each page for
	// rendering. This approach means that sometimes, content providers for
	// translations are not initialized. If needed, initialize them here.
	for _, tr := range p.allTranslations {
		tp, ok := tr.(*pageState)
		if !ok {
			continue
		}
		if tp.pageOutput.cp == nil {
			cp, err := newPageContentOutput(tp, tp.pageOutput)
			if err != nil {
				// This will be caught by texttemplate.safeCall
				panic(fmt.Errorf("error rendering page translation: %w", err))
			}
			tp.initContentProvider(cp)
		}
	}

	return p.allTranslations
}

[ptgott]

@bep I've suggested initializing a LazyContentProvider inside RenderString. If this approach looks okay, should I go ahead and see if other *pageState methods need a similar treatment?

[bep]

So, this is conceptually what we need, but I would love if we could do this in one place. I think that if we use the same "interface approach" for RenderString (and potentially others) and make LazyContentProvider implement that interface, that should work.

[bep]

I have some time today and will look closer at this PR -- again, much appreciated.

[bep]

@ptgott I have added a commit to your PR which is a little bit more elaborate, but it's just moving the RenderString and Render code down to where it belongs, I guess. Note that there may still be cases where .Translations[0].SomeMethod may still return empty (I have always thought about the translation listings as mostly informative/metadata), but at least now it's not clearly specificed by an interface (OutputFormatContentProvider).

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.