Skip to content

Commit 8ec3a6f

Browse files
committed
KUBEDR-7763: Don't panic if CanShareWork() is called on a closed work pool
1 parent f771119 commit 8ec3a6f

6 files changed

Lines changed: 18 additions & 11 deletions

File tree

internal/workshare/workshare_pool.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
package workshare
33

44
import (
5+
"context"
56
"sync"
67
"sync/atomic"
78
)
@@ -21,6 +22,7 @@ type workItem[T any] struct {
2122
type Pool[T any] struct {
2223
activeWorkers atomic.Int32
2324

25+
ctx context.Context
2426
semaphore chan struct{}
2527

2628
work chan workItem[T]
@@ -35,12 +37,13 @@ func (w *Pool[T]) ActiveWorkers() int {
3537
}
3638

3739
// NewPool creates a worker pool that launches a given number of goroutines that can invoke shared work.
38-
func NewPool[T any](numWorkers int) *Pool[T] {
40+
func NewPool[T any](ctx context.Context, numWorkers int) *Pool[T] {
3941
if numWorkers < 0 {
4042
numWorkers = 0
4143
}
4244

4345
w := &Pool[T]{
46+
ctx: ctx,
4447
// channel must be unbuffered so that it has exactly as many slots as there are goroutines capable of reading from it
4548
// this way by pushing to the channel we can be sure that a pre-spun goroutine will pick it up soon.
4649
work: make(chan workItem[T]),

internal/workshare/workshare_test.go

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

33
import (
4+
"context"
45
"testing"
56

67
"github.com/stretchr/testify/require"
@@ -119,15 +120,13 @@ func TestDisallowed_WaitAfterClose(t *testing.T) {
119120
}
120121

121122
func TestDisallowed_UseAfterPoolClose(t *testing.T) {
122-
w := workshare.NewPool[int](1)
123+
w := workshare.NewPool[int](context.Background(), 1)
123124

124125
var ag workshare.AsyncGroup[int]
125126

126127
w.Close()
127128

128-
require.Panics(t, func() {
129-
ag.CanShareWork(w)
130-
})
129+
require.False(t, ag.CanShareWork(w))
131130

132131
require.Panics(t, func() {
133132
ag.RunAsync(w, func(c *workshare.Pool[int], request int) {
@@ -138,7 +137,7 @@ func TestDisallowed_UseAfterPoolClose(t *testing.T) {
138137

139138
//nolint:thelper
140139
func testComputeTreeSum(t *testing.T, numWorkers int) {
141-
w := workshare.NewPool[*computeTreeSumRequest](numWorkers)
140+
w := workshare.NewPool[*computeTreeSumRequest](context.Background(), numWorkers)
142141
defer w.Close()
143142

144143
n := buildTree(6)
@@ -151,7 +150,7 @@ func testComputeTreeSum(t *testing.T, numWorkers int) {
151150
var treeToWalk = buildTree(6)
152151

153152
func BenchmarkComputeTreeSum(b *testing.B) {
154-
w := workshare.NewPool[*computeTreeSumRequest](10)
153+
w := workshare.NewPool[*computeTreeSumRequest](context.Background(), 10)
155154
defer w.Close()
156155

157156
b.ResetTimer()

internal/workshare/workshare_waitgroup.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,12 @@ package workshare
4444

4545
import (
4646
"sync"
47+
48+
"github.com/kopia/kopia/repo/logging"
4749
)
4850

51+
var log = logging.Module("workshare")
52+
4953
// AsyncGroup launches and awaits asynchronous work through a WorkerPool.
5054
// It provides API designed to minimize allocations while being reasonably easy to use.
5155
// AsyncGroup is very lightweight and NOT safe for concurrent use, all interactions must
@@ -121,7 +125,8 @@ func (g *AsyncGroup[T]) CanShareWork(w *Pool[T]) bool {
121125
// check pool is not closed
122126
select {
123127
case <-w.closed:
124-
panic("invalid usage - CanShareWork() after workshare.AsyncGroup has been closed")
128+
log(w.ctx).Warn("CanShareWork() is called after workshare.Pool has been closed")
129+
return false
125130

126131
default:
127132
select {

snapshot/snapshotfs/dir_rewriter.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,7 @@ func NewDirRewriter(ctx context.Context, rep repo.RepositoryWriter, opts DirRewr
338338
}
339339

340340
return &DirRewriter{
341-
ws: workshare.NewPool[*dirRewriterRequest](opts.Parallel - 1),
341+
ws: workshare.NewPool[*dirRewriterRequest](ctx, opts.Parallel-1),
342342
opts: opts,
343343
rep: rep,
344344
cache: cache,

snapshot/snapshotfs/snapshot_tree_walker.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ func NewTreeWalker(ctx context.Context, options TreeWalkerOptions) (*TreeWalker,
193193

194194
return &TreeWalker{
195195
options: options,
196-
wp: workshare.NewPool[any](options.Parallelism - 1),
196+
wp: workshare.NewPool[any](ctx, options.Parallelism-1),
197197
enqueued: s,
198198
}, nil
199199
}

snapshot/snapshotfs/upload.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1267,7 +1267,7 @@ func (u *Uploader) Upload(
12671267
Source: sourceInfo,
12681268
}
12691269

1270-
u.workerPool = workshare.NewPool[*uploadWorkItem](parallel - 1)
1270+
u.workerPool = workshare.NewPool[*uploadWorkItem](ctx, parallel-1)
12711271
defer u.workerPool.Close()
12721272

12731273
u.stats = &snapshot.Stats{}

0 commit comments

Comments
 (0)