]> git.maquefel.me Git - brevno-suite/hugo/commitdiff
Fix concurrent map read and map write in short page lookups
authorBjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com>
Wed, 6 Nov 2024 09:17:34 +0000 (10:17 +0100)
committerBjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com>
Wed, 6 Nov 2024 11:17:30 +0000 (12:17 +0100)
Regression introduced in Hugo `v0.137.0`.

Fixes #13019

common/maps/cache.go
hugolib/content_map_page.go
hugolib/content_map_test.go
lazy/init.go
lazy/once.go

index 0175974b5b4517ab4db1413ea4d9743b85ef5d83..cdc31a68467c23933160f54b07cc9ae1b9ec8873 100644 (file)
 
 package maps
 
-import "sync"
+import (
+       "sync"
+)
 
 // Cache is a simple thread safe cache backed by a map.
 type Cache[K comparable, T any] struct {
-       m map[K]T
+       m                  map[K]T
+       hasBeenInitialized bool
        sync.RWMutex
 }
 
@@ -34,11 +37,16 @@ func (c *Cache[K, T]) Get(key K) (T, bool) {
                return zero, false
        }
        c.RLock()
-       v, found := c.m[key]
+       v, found := c.get(key)
        c.RUnlock()
        return v, found
 }
 
+func (c *Cache[K, T]) get(key K) (T, bool) {
+       v, found := c.m[key]
+       return v, found
+}
+
 // GetOrCreate gets the value for the given key if it exists, or creates it if not.
 func (c *Cache[K, T]) GetOrCreate(key K, create func() (T, error)) (T, error) {
        c.RLock()
@@ -61,13 +69,49 @@ func (c *Cache[K, T]) GetOrCreate(key K, create func() (T, error)) (T, error) {
        return v, nil
 }
 
+// InitAndGet initializes the cache if not already done and returns the value for the given key.
+// The init state will be reset on Reset or Drain.
+func (c *Cache[K, T]) InitAndGet(key K, init func(get func(key K) (T, bool), set func(key K, value T)) error) (T, error) {
+       var v T
+       c.RLock()
+       if !c.hasBeenInitialized {
+               c.RUnlock()
+               if err := func() error {
+                       c.Lock()
+                       defer c.Unlock()
+                       // Double check in case another goroutine has initialized it in the meantime.
+                       if !c.hasBeenInitialized {
+                               err := init(c.get, c.set)
+                               if err != nil {
+                                       return err
+                               }
+                               c.hasBeenInitialized = true
+                       }
+                       return nil
+               }(); err != nil {
+                       return v, err
+               }
+               // Reacquire the read lock.
+               c.RLock()
+       }
+
+       v = c.m[key]
+       c.RUnlock()
+
+       return v, nil
+}
+
 // Set sets the given key to the given value.
 func (c *Cache[K, T]) Set(key K, value T) {
        c.Lock()
-       c.m[key] = value
+       c.set(key, value)
        c.Unlock()
 }
 
+func (c *Cache[K, T]) set(key K, value T) {
+       c.m[key] = value
+}
+
 // ForEeach calls the given function for each key/value pair in the cache.
 func (c *Cache[K, T]) ForEeach(f func(K, T)) {
        c.RLock()
@@ -81,6 +125,7 @@ func (c *Cache[K, T]) Drain() map[K]T {
        c.Lock()
        m := c.m
        c.m = make(map[K]T)
+       c.hasBeenInitialized = false
        c.Unlock()
        return m
 }
@@ -94,6 +139,7 @@ func (c *Cache[K, T]) Len() int {
 func (c *Cache[K, T]) Reset() {
        c.Lock()
        c.m = make(map[K]T)
+       c.hasBeenInitialized = false
        c.Unlock()
 }
 
index 5e8646b21c464fd442b61c1a49ee18e65a8f3658..8c9e4a31a2ff00bef282cc8a3797ea296f41cf49 100644 (file)
@@ -37,7 +37,6 @@ import (
        "github.com/gohugoio/hugo/hugolib/doctree"
        "github.com/gohugoio/hugo/hugolib/pagesfromdata"
        "github.com/gohugoio/hugo/identity"
-       "github.com/gohugoio/hugo/lazy"
        "github.com/gohugoio/hugo/media"
        "github.com/gohugoio/hugo/output"
        "github.com/gohugoio/hugo/resources"
@@ -925,59 +924,58 @@ func newPageMap(i int, s *Site, mcache *dynacache.Cache, pageTrees *pageTrees) *
                s: s,
        }
 
-       m.pageReverseIndex = &contentTreeReverseIndex{
-               initFn: func(rm map[any]contentNodeI) {
-                       add := func(k string, n contentNodeI) {
-                               existing, found := rm[k]
-                               if found && existing != ambiguousContentNode {
-                                       rm[k] = ambiguousContentNode
-                               } else if !found {
-                                       rm[k] = n
-                               }
+       m.pageReverseIndex = newContentTreeTreverseIndex(func(get func(key any) (contentNodeI, bool), set func(key any, val contentNodeI)) {
+               add := func(k string, n contentNodeI) {
+                       existing, found := get(k)
+                       if found && existing != ambiguousContentNode {
+                               set(k, ambiguousContentNode)
+                       } else if !found {
+                               set(k, n)
                        }
+               }
 
-                       w := &doctree.NodeShiftTreeWalker[contentNodeI]{
-                               Tree:     m.treePages,
-                               LockType: doctree.LockTypeRead,
-                               Handle: func(s string, n contentNodeI, match doctree.DimensionFlag) (bool, error) {
-                                       p := n.(*pageState)
-                                       if p.PathInfo() != nil {
-                                               add(p.PathInfo().BaseNameNoIdentifier(), p)
-                                       }
-                                       return false, nil
-                               },
-                       }
+               w := &doctree.NodeShiftTreeWalker[contentNodeI]{
+                       Tree:     m.treePages,
+                       LockType: doctree.LockTypeRead,
+                       Handle: func(s string, n contentNodeI, match doctree.DimensionFlag) (bool, error) {
+                               p := n.(*pageState)
+                               if p.PathInfo() != nil {
+                                       add(p.PathInfo().BaseNameNoIdentifier(), p)
+                               }
+                               return false, nil
+                       },
+               }
 
-                       if err := w.Walk(context.Background()); err != nil {
-                               panic(err)
-                       }
-               },
-               contentTreeReverseIndexMap: &contentTreeReverseIndexMap{},
-       }
+               if err := w.Walk(context.Background()); err != nil {
+                       panic(err)
+               }
+       })
 
        return m
 }
 
+func newContentTreeTreverseIndex(init func(get func(key any) (contentNodeI, bool), set func(key any, val contentNodeI))) *contentTreeReverseIndex {
+       return &contentTreeReverseIndex{
+               initFn: init,
+               mm:     maps.NewCache[any, contentNodeI](),
+       }
+}
+
 type contentTreeReverseIndex struct {
-       initFn func(rm map[any]contentNodeI)
-       *contentTreeReverseIndexMap
+       initFn func(get func(key any) (contentNodeI, bool), set func(key any, val contentNodeI))
+       mm     *maps.Cache[any, contentNodeI]
 }
 
 func (c *contentTreeReverseIndex) Reset() {
-       c.init.ResetWithLock().Unlock()
+       c.mm.Reset()
 }
 
 func (c *contentTreeReverseIndex) Get(key any) contentNodeI {
-       c.init.Do(func() {
-               c.m = make(map[any]contentNodeI)
-               c.initFn(c.contentTreeReverseIndexMap.m)
+       v, _ := c.mm.InitAndGet(key, func(get func(key any) (contentNodeI, bool), set func(key any, val contentNodeI)) error {
+               c.initFn(get, set)
+               return nil
        })
-       return c.m[key]
-}
-
-type contentTreeReverseIndexMap struct {
-       init lazy.OnceMore
-       m    map[any]contentNodeI
+       return v
 }
 
 type sitePagesAssembler struct {
index bf9920071718b7c310b7f458cd317974ec212462..a9f719f4a835b63eaebb73530eef798a46b8571f 100644 (file)
@@ -17,9 +17,11 @@ import (
        "fmt"
        "path/filepath"
        "strings"
+       "sync"
        "testing"
 
        qt "github.com/frankban/quicktest"
+       "github.com/gohugoio/hugo/identity"
 )
 
 func TestContentMapSite(t *testing.T) {
@@ -396,3 +398,77 @@ irrelevant
                "<loc>https://example.org/en/sitemap.xml</loc>",
        )
 }
+
+func TestContentTreeReverseIndex(t *testing.T) {
+       t.Parallel()
+
+       c := qt.New(t)
+
+       pageReverseIndex := newContentTreeTreverseIndex(
+               func(get func(key any) (contentNodeI, bool), set func(key any, val contentNodeI)) {
+                       for i := 0; i < 10; i++ {
+                               key := fmt.Sprint(i)
+                               set(key, &testContentNode{key: key})
+                       }
+               },
+       )
+
+       for i := 0; i < 10; i++ {
+               key := fmt.Sprint(i)
+               v := pageReverseIndex.Get(key)
+               c.Assert(v, qt.Not(qt.IsNil))
+               c.Assert(v.Path(), qt.Equals, key)
+       }
+}
+
+// Issue 13019.
+func TestContentTreeReverseIndexPara(t *testing.T) {
+       t.Parallel()
+
+       var wg sync.WaitGroup
+
+       for i := 0; i < 10; i++ {
+               pageReverseIndex := newContentTreeTreverseIndex(
+                       func(get func(key any) (contentNodeI, bool), set func(key any, val contentNodeI)) {
+                               for i := 0; i < 10; i++ {
+                                       key := fmt.Sprint(i)
+                                       set(key, &testContentNode{key: key})
+                               }
+                       },
+               )
+
+               for j := 0; j < 10; j++ {
+                       wg.Add(1)
+                       go func(i int) {
+                               defer wg.Done()
+                               pageReverseIndex.Get(fmt.Sprint(i))
+                       }(j)
+               }
+       }
+}
+
+type testContentNode struct {
+       key string
+}
+
+func (n *testContentNode) GetIdentity() identity.Identity {
+       return identity.StringIdentity(n.key)
+}
+
+func (n *testContentNode) ForEeachIdentity(cb func(id identity.Identity) bool) bool {
+       panic("not supported")
+}
+
+func (n *testContentNode) Path() string {
+       return n.key
+}
+
+func (n *testContentNode) isContentNodeBranch() bool {
+       return false
+}
+
+func (n *testContentNode) resetBuildState() {
+}
+
+func (n *testContentNode) MarkStale() {
+}
index bef3867a9a0c7535a0645a50e97ea2a5974442dc..7b88a53518be3b72c715ac7dc609e292ef8cf50e 100644 (file)
@@ -36,7 +36,7 @@ type Init struct {
        prev     *Init
        children []*Init
 
-       init OnceMore
+       init onceMore
        out  any
        err  error
        f    func(context.Context) (any, error)
index dac689df393644252acadcc68cc5a335ea8d510a..cea0966523b5dfea179c2cfac4f0c4b68c5ded7f 100644 (file)
@@ -24,13 +24,13 @@ import (
 // * it can be reset, so the action can be repeated if needed
 // * it has methods to check if it's done or in progress
 
-type OnceMore struct {
+type onceMore struct {
        mu   sync.Mutex
        lock uint32
        done uint32
 }
 
-func (t *OnceMore) Do(f func()) {
+func (t *onceMore) Do(f func()) {
        if atomic.LoadUint32(&t.done) == 1 {
                return
        }
@@ -53,15 +53,15 @@ func (t *OnceMore) Do(f func()) {
        f()
 }
 
-func (t *OnceMore) InProgress() bool {
+func (t *onceMore) InProgress() bool {
        return atomic.LoadUint32(&t.lock) == 1
 }
 
-func (t *OnceMore) Done() bool {
+func (t *onceMore) Done() bool {
        return atomic.LoadUint32(&t.done) == 1
 }
 
-func (t *OnceMore) ResetWithLock() *sync.Mutex {
+func (t *onceMore) ResetWithLock() *sync.Mutex {
        t.mu.Lock()
        defer atomic.StoreUint32(&t.done, 0)
        return &t.mu