From 94a6233aabcad831041f1409aa07d45065eb817f Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bj=C3=B8rn=20Erik=20Pedersen?= Date: Sat, 15 Nov 2025 11:40:18 +0100 Subject: [PATCH] hugolib: Fix recently introduced data race This introduces a new thread safe maps.Map type to avoid abusing the exsting maps.Cache for regular map uses. Fixes #14140 --- common/maps/cache_test.go | 2 +- common/maps/map.go | 117 ++++++++++++++++++++++++++ common/maps/map_test.go | 71 ++++++++++++++++ hugolib/content_map_page_assembler.go | 55 ++++++------ hugolib/doctree/nodeshifttree.go | 18 +++- hugolib/hugo_sites.go | 4 +- 6 files changed, 236 insertions(+), 31 deletions(-) create mode 100644 common/maps/map.go create mode 100644 common/maps/map_test.go diff --git a/common/maps/cache_test.go b/common/maps/cache_test.go index 32bff9572..239c56fec 100644 --- a/common/maps/cache_test.go +++ b/common/maps/cache_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 The Hugo Authors. All rights reserved. +// Copyright 2025 The Hugo Authors. All rights reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/common/maps/map.go b/common/maps/map.go new file mode 100644 index 000000000..d1db71a88 --- /dev/null +++ b/common/maps/map.go @@ -0,0 +1,117 @@ +// Copyright 2025 The Hugo Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package maps + +import ( + "iter" + "sync" +) + +func NewMap[K comparable, T any]() *Map[K, T] { + return &Map[K, T]{ + m: make(map[K]T), + } +} + +// Map is a thread safe map backed by a Go map. +type Map[K comparable, T any] struct { + m map[K]T + mu sync.RWMutex +} + +// Get gets the value for the given key. +// It returns the zero value of T if the key is not found. +func (m *Map[K, T]) Get(key K) T { + v, _ := m.Lookup(key) + return v +} + +// Lookup looks up the given key in the map. +// It returns the value and a boolean indicating whether the key was found. +func (m *Map[K, T]) Lookup(key K) (T, bool) { + m.mu.RLock() + v, found := m.m[key] + m.mu.RUnlock() + return v, found +} + +// GetOrCreate gets the value for the given key if it exists, or creates it if not. +func (m *Map[K, T]) GetOrCreate(key K, create func() (T, error)) (T, error) { + v, found := m.Lookup(key) + if found { + return v, nil + } + m.mu.Lock() + defer m.mu.Unlock() + v, found = m.m[key] + if found { + return v, nil + } + v, err := create() + if err != nil { + return v, err + } + m.m[key] = v + return v, nil +} + +// Set sets the given key to the given value. +func (m *Map[K, T]) Set(key K, value T) { + m.mu.Lock() + m.m[key] = value + m.mu.Unlock() +} + +// WithWriteLock executes the given function with a write lock on the map. +func (m *Map[K, T]) WithWriteLock(f func(m map[K]T)) { + m.mu.Lock() + defer m.mu.Unlock() + f(m.m) +} + +// SetIfAbsent sets the given key to the given value if the key does not already exist in the map. +// It returns true if the value was set, false otherwise. +func (m *Map[K, T]) SetIfAbsent(key K, value T) bool { + m.mu.RLock() + if _, found := m.m[key]; !found { + m.mu.RUnlock() + return m.doSetIfAbsent(key, value) + } + m.mu.RUnlock() + return false +} + +func (m *Map[K, T]) doSetIfAbsent(key K, value T) bool { + m.mu.Lock() + defer m.mu.Unlock() + if _, found := m.m[key]; !found { + m.m[key] = value + return true + } + return false +} + +// All returns an iterator over all key/value pairs in the map. +// A read lock is held during the iteration. +func (m *Map[K, T]) All() iter.Seq2[K, T] { + return func(yield func(K, T) bool) { + m.mu.RLock() + defer m.mu.RUnlock() + for k, v := range m.m { + if !yield(k, v) { + return + } + } + } +} diff --git a/common/maps/map_test.go b/common/maps/map_test.go new file mode 100644 index 000000000..55246a2f2 --- /dev/null +++ b/common/maps/map_test.go @@ -0,0 +1,71 @@ +// Copyright 2025 The Hugo Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package maps + +import ( + "testing" + + qt "github.com/frankban/quicktest" +) + +func TestMap(t *testing.T) { + c := qt.New(t) + + m := NewMap[string, int]() + + m.Set("b", 42) + v, found := m.Lookup("b") + c.Assert(found, qt.Equals, true) + c.Assert(v, qt.Equals, 42) + v = m.Get("b") + c.Assert(v, qt.Equals, 42) + v, found = m.Lookup("c") + c.Assert(found, qt.Equals, false) + c.Assert(v, qt.Equals, 0) + v = m.Get("c") + c.Assert(v, qt.Equals, 0) + v, err := m.GetOrCreate("d", func() (int, error) { + return 100, nil + }) + c.Assert(err, qt.IsNil) + c.Assert(v, qt.Equals, 100) + v, found = m.Lookup("d") + c.Assert(found, qt.Equals, true) + c.Assert(v, qt.Equals, 100) + + v, err = m.GetOrCreate("d", func() (int, error) { + return 200, nil + }) + c.Assert(err, qt.IsNil) + c.Assert(v, qt.Equals, 100) + + wasSet := m.SetIfAbsent("e", 300) + c.Assert(wasSet, qt.Equals, true) + v, found = m.Lookup("e") + c.Assert(found, qt.Equals, true) + c.Assert(v, qt.Equals, 300) + + wasSet = m.SetIfAbsent("e", 400) + c.Assert(wasSet, qt.Equals, false) + v, found = m.Lookup("e") + c.Assert(found, qt.Equals, true) + c.Assert(v, qt.Equals, 300) + + m.WithWriteLock(func(m map[string]int) { + m["f"] = 500 + }) + v, found = m.Lookup("f") + c.Assert(found, qt.Equals, true) + c.Assert(v, qt.Equals, 500) +} diff --git a/hugolib/content_map_page_assembler.go b/hugolib/content_map_page_assembler.go index 47e03d782..7396ec5e4 100644 --- a/hugolib/content_map_page_assembler.go +++ b/hugolib/content_map_page_assembler.go @@ -48,15 +48,16 @@ type allPagesAssembler struct { r para.Runner assembleChanges *WhatChanged - seenTerms map[term]sitesmatrix.Vectors - droppedPages map[*Site][]string // e.g. drafts, expired, future. - seenRootSections *maps.Cache[string, bool] - seenHome bool assembleSectionsInParallel bool - // Internal state. pwRoot *doctree.NodeShiftTreeWalker[contentNode] // walks pages. rwRoot *doctree.NodeShiftTreeWalker[contentNode] // walks resources. + + // Walking state. + seenTerms *maps.Map[term, sitesmatrix.Vectors] + droppedPages *maps.Map[*Site, []string] // e.g. drafts, expired, future. + seenRootSections *maps.Map[string, bool] + seenHome bool // set before we fan out to multiple goroutines. } func newAllPagesAssembler( @@ -74,7 +75,7 @@ func newAllPagesAssembler( pw := rw.Extend() pw.Tree = m.treePages - seenRootSections := maps.NewCache[string, bool]() + seenRootSections := maps.NewMap[string, bool]() seenRootSections.Set("", true) // home. return &allPagesAssembler{ @@ -82,8 +83,8 @@ func newAllPagesAssembler( h: h, m: m, assembleChanges: assembleChanges, - seenTerms: map[term]sitesmatrix.Vectors{}, - droppedPages: map[*Site][]string{}, + seenTerms: maps.NewMap[term, sitesmatrix.Vectors](), + droppedPages: maps.NewMap[*Site, []string](), seenRootSections: seenRootSections, assembleSectionsInParallel: true, pwRoot: pw, @@ -100,7 +101,7 @@ type sitePagesAssembler struct { func (a *allPagesAssembler) createAllPages() error { defer func() { - for site, dropped := range a.droppedPages { + for site, dropped := range a.droppedPages.All() { for _, s := range dropped { site.pageMap.treePages.Delete(s) site.pageMap.resourceTrees.DeletePrefix(paths.AddTrailingSlash(s)) @@ -112,16 +113,16 @@ func (a *allPagesAssembler) createAllPages() error { defer func() { if a.h.isRebuild() && a.h.previousSeenTerms != nil { // Find removed terms. - for t := range a.h.previousSeenTerms { - if _, found := a.seenTerms[t]; !found { + for t := range a.h.previousSeenTerms.All() { + if _, found := a.seenTerms.Lookup(t); !found { // This term has been removed. a.pwRoot.Tree.Delete(t.view.pluralTreeKey) a.rwRoot.Tree.DeletePrefix(t.view.pluralTreeKey + "/") } } // Find new terms. - for t := range a.seenTerms { - if _, found := a.h.previousSeenTerms[t]; !found { + for t := range a.seenTerms.All() { + if _, found := a.h.previousSeenTerms.Lookup(t); !found { // This term is new. if n, ok := a.pwRoot.Tree.GetRaw(t.view.pluralTreeKey); ok { a.assembleChanges.Add(cnh.GetIdentity(n)) @@ -284,7 +285,12 @@ func (a *allPagesAssembler) doCreatePages(prefix string) error { (&p.m.pageConfig.Build).Disable() default: // Skip this page. - a.droppedPages[site] = append(a.droppedPages[site], v.Path()) + a.droppedPages.WithWriteLock( + func(m map[*Site][]string) { + m[site] = append(m[site], s) + }, + ) + drop = true } } @@ -471,13 +477,13 @@ func (a *allPagesAssembler) doCreatePages(prefix string) error { } if !isResource && s != "" && !a.seenHome { + a.seenHome = true var homePages contentNode homePages, ns, err = transformPages("", newHomePageMetaSource(), cascades) if err != nil { return } treePages.InsertRaw("", homePages) - a.seenHome = true } n2, ns, err = transformPages(s, n, cascades) @@ -633,17 +639,16 @@ func (a *allPagesAssembler) doCreatePages(prefix string) error { continue } t := term{view: viewName, term: v} - if vectors, found := a.seenTerms[t]; found { - if _, found := vectors[vec]; found { - continue + a.seenTerms.WithWriteLock(func(m map[term]sitesmatrix.Vectors) { + vectors, found := m[t] + if !found { + m[t] = sitesmatrix.Vectors{ + vec: struct{}{}, + } + return } vectors[vec] = struct{}{} - } else { - a.seenTerms[t] = sitesmatrix.Vectors{ - vec: struct{}{}, - } - } - + }) } } } @@ -752,7 +757,7 @@ func (a *allPagesAssembler) doCreatePages(prefix string) error { // Create missing term pages. pw.WalkContext.HooksPost2().Push( func() error { - for k, v := range a.seenTerms { + for k, v := range a.seenTerms.All() { viewTermKey := "/" + k.view.plural + "/" + k.term pi := a.h.Conf.PathParser().Parse(files.ComponentFolderContent, viewTermKey+"/_index.md") diff --git a/hugolib/doctree/nodeshifttree.go b/hugolib/doctree/nodeshifttree.go index 8f64be396..0ab7ed121 100644 --- a/hugolib/doctree/nodeshifttree.go +++ b/hugolib/doctree/nodeshifttree.go @@ -429,9 +429,21 @@ type NodeShiftTreeWalker[T any] struct { // Extend returns a new NodeShiftTreeWalker with the same configuration // and the same WalkContext as the original. // Any local state is reset. -func (r NodeShiftTreeWalker[T]) Extend() *NodeShiftTreeWalker[T] { - r.skipPrefixes = nil - return &r +func (r *NodeShiftTreeWalker[T]) Extend() *NodeShiftTreeWalker[T] { + return &NodeShiftTreeWalker[T]{ + Tree: r.Tree, + Transform: r.Transform, + TransformDelayInsert: r.TransformDelayInsert, + Handle: r.Handle, + Prefix: r.Prefix, + IncludeFilter: r.IncludeFilter, + IncludeRawFilter: r.IncludeRawFilter, + LockType: r.LockType, + NoShift: r.NoShift, + Fallback: r.Fallback, + Debug: r.Debug, + WalkContext: r.WalkContext, + } } // WithPrefix returns a new NodeShiftTreeWalker with the given prefix. diff --git a/hugolib/hugo_sites.go b/hugolib/hugo_sites.go index b1ac8fb25..a01f93b75 100644 --- a/hugolib/hugo_sites.go +++ b/hugolib/hugo_sites.go @@ -96,8 +96,8 @@ type HugoSites struct { translationKeyPages *maps.SliceCache[page.Page] pageTrees *pageTrees - previousPageTreesWalkContext *doctree.WalkContext[contentNode] // Set for rebuilds only. - previousSeenTerms map[term]sitesmatrix.Vectors // Set for rebuilds only. + previousPageTreesWalkContext *doctree.WalkContext[contentNode] // Set for rebuilds only. + previousSeenTerms *maps.Map[term, sitesmatrix.Vectors] // Set for rebuilds only. printUnusedTemplatesInit sync.Once printPathWarningsInit sync.Once -- 2.39.5