Skip to content

Commit 24e43a4

Browse files
committed
Fix handling of panics in GetOrCreate
One could argue that the func passed in should never panic, but before this commit this could easily lead to deadlocks.
1 parent b9b1303 commit 24e43a4

File tree

2 files changed

+39
-11
lines changed

2 files changed

+39
-11
lines changed

‎lazycache.go‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package lazycache
22

33
import (
4+
"fmt"
45
"sync"
56

67
"github.com/hashicorp/golang-lru/v2/simplelru"
@@ -121,19 +122,28 @@ func (c *Cache[K, V]) GetOrCreate(key K, create func(key K) (V, error)) (V, bool
121122
c.lru.Add(key, w)
122123
c.mu.Unlock()
123124

124-
// Create the value with the lock released.
125-
v, err := create(key)
126-
w.err = err
127-
w.value = v
128-
w.found = err == nil
125+
v, err := func() (v V, err error) {
126+
defer func() {
127+
if r := recover(); r != nil {
128+
err = fmt.Errorf("panic: %v", r)
129+
}
130+
w.err = err
131+
w.value = v
132+
w.found = err == nil
133+
close(w.ready)
134+
135+
if err != nil {
136+
v = c.zerov
137+
c.Delete(key)
138+
}
139+
}()
140+
// Create the value with the lock released.
141+
v, err = create(key)
129142

130-
close(w.ready)
143+
return
144+
}()
131145

132-
if err != nil {
133-
c.Delete(key)
134-
return c.zerov, false, err
135-
}
136-
return v, false, nil
146+
return v, false, err
137147
}
138148

139149
// Resize changes the cache size and returns the number of entries evicted.

‎lazycache_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,24 @@ func TestCache(t *testing.T) {
5757
c.Assert(func() { New[int, any](Options[int, any]{MaxEntries: -1}) }, qt.PanicMatches, "must provide a positive size")
5858
}
5959

60+
func TestPanic(t *testing.T) {
61+
c := qt.New(t)
62+
63+
cache := New(Options[int, any]{MaxEntries: 1000})
64+
for i := 0; i < 2; i++ {
65+
_, _, err := cache.GetOrCreate(42, func(key int) (any, error) {
66+
panic("failed1")
67+
})
68+
c.Assert(err, qt.IsNotNil)
69+
c.Assert(err, qt.ErrorMatches, "panic: failed1")
70+
_, _, err = cache.GetOrCreate(i, func(key int) (any, error) {
71+
panic("failed2")
72+
})
73+
c.Assert(err, qt.IsNotNil)
74+
c.Assert(err, qt.ErrorMatches, "panic: failed2")
75+
}
76+
}
77+
6078
func TestDeleteFunc(t *testing.T) {
6179
c := qt.New(t)
6280

0 commit comments

Comments
 (0)