From 10753a7bbf1177905fc8c36db5199573ece971be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20Erik=20Pedersen?= Date: Wed, 2 Mar 2022 10:04:29 +0100 Subject: [PATCH] tpl/partials: Fix partialCached deadlock regression This is a rollback of 0927cf739fee9646c7fb917965799d9acf080922 We cannot do that change until we either completes #9570 or possibly also use the new TryLock in GO 1.18. Fixes #9588 Opens #4086 --- tpl/partials/integration_test.go | 38 ++++++++++++++++++++++++++++++++ tpl/partials/partials.go | 38 ++++++++++++++++---------------- 2 files changed, 57 insertions(+), 19 deletions(-) diff --git a/tpl/partials/integration_test.go b/tpl/partials/integration_test.go index 446e4711809..bda5ddbd5c6 100644 --- a/tpl/partials/integration_test.go +++ b/tpl/partials/integration_test.go @@ -103,6 +103,44 @@ P2 `) } +// Issue #588 +func TestIncludeCachedRecursionShortcode(t *testing.T) { + t.Parallel() + + files := ` +-- config.toml -- +baseURL = 'http://example.com/' +-- content/_index.md -- +--- +title: "Index" +--- +{{< short >}} +-- layouts/index.html -- +{{ partials.IncludeCached "p1.html" . }} +-- layouts/partials/p1.html -- +{{ .Content }} +{{ partials.IncludeCached "p2.html" . }} +-- layouts/partials/p2.html -- +-- layouts/shortcodes/short.html -- +SHORT +{{ partials.IncludeCached "p2.html" . }} +P2 + + ` + + b := hugolib.NewIntegrationTestBuilder( + hugolib.IntegrationTestConfig{ + T: t, + TxtarString: files, + }, + ).Build() + + b.AssertFileContent("public/index.html", ` +SHORT +P2 +`) +} + func TestIncludeCacheHints(t *testing.T) { t.Parallel() diff --git a/tpl/partials/partials.go b/tpl/partials/partials.go index 500f5d1a358..b18e280ca4b 100644 --- a/tpl/partials/partials.go +++ b/tpl/partials/partials.go @@ -233,21 +233,14 @@ func (ns *Namespace) getOrCreate(ctx context.Context, key partialCacheKey, conte } }() - // We may already have a write lock. - hasLock := tpl.GetHasLockFromContext(ctx) - - if !hasLock { - ns.cachedPartials.RLock() - } + ns.cachedPartials.RLock() p, ok := ns.cachedPartials.p[key] - if !hasLock { - ns.cachedPartials.RUnlock() - } + ns.cachedPartials.RUnlock() if ok { if ns.deps.Metrics != nil { ns.deps.Metrics.TrackValue(key.templateName(), p, true) - // The templates that gets executed is measued in Execute. + // The templates that gets executed is measured in Execute. // We need to track the time spent in the cache to // get the totals correct. ns.deps.Metrics.MeasureSince(key.templateName(), start) @@ -256,21 +249,28 @@ func (ns *Namespace) getOrCreate(ctx context.Context, key partialCacheKey, conte return p, nil } - if !hasLock { - ns.cachedPartials.Lock() - defer ns.cachedPartials.Unlock() - ctx = tpl.SetHasLockInContext(ctx, true) - } - - var name string - name, p, err = ns.include(ctx, key.name, context) + // This needs to be done outside the lock. + // See #9588 + _, p, err = ns.include(ctx, key.name, context) if err != nil { return nil, err } + ns.cachedPartials.Lock() + defer ns.cachedPartials.Unlock() + // Double-check. + if p2, ok := ns.cachedPartials.p[key]; ok { + if ns.deps.Metrics != nil { + ns.deps.Metrics.TrackValue(key.templateName(), p, true) + ns.deps.Metrics.MeasureSince(key.templateName(), start) + } + return p2, nil + + } if ns.deps.Metrics != nil { - ns.deps.Metrics.TrackValue(name, p, false) + ns.deps.Metrics.TrackValue(key.templateName(), p, false) } + ns.cachedPartials.p[key] = p return p, nil