Skip to content

Commit 2b4ed9b

Browse files
refactor: replace doRevalFn closure with direct filter reference in revalJob
Replace the func() closure field on revalJob with a *cacheFilter pointer and call job.filter.doRevalidate(job.key, job.req) directly in the worker. The closure was only binding key and req, which revalJob already carries as fields, making the indirection unnecessary. Also fixes two missing spec.Close() calls in TestCreateFilter_InvalidArgs and TestCacheFilter_SharedStorage_RouteIsolation that caused goroutine leaks. Signed-off-by: larry-dalmeida <hello@larrydalmeida.com>
1 parent 4a7f535 commit 2b4ed9b

2 files changed

Lines changed: 14 additions & 13 deletions

File tree

filters/cache/filter.go

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -202,12 +202,12 @@ func (s *cacheSpec) CreateFilter(args []interface{}) (filters.Filter, error) {
202202

203203
// revalidationWorker is the single background goroutine (spec-level, shared across
204204
// all filter instances) that processes revalidation jobs sequentially. It calls
205-
// the per-instance doRevalidateFn closure to respect each route's configuration.
205+
// the per-instance doRevalidate method to respect each route's configuration.
206206
func (s *cacheSpec) revalidationWorker() {
207207
defer s.bgWg.Done()
208208
for job := range s.revalJobs {
209-
if job.doRevalFn != nil {
210-
job.doRevalFn()
209+
if job.filter != nil {
210+
job.filter.doRevalidate(job.key, job.req)
211211
}
212212
}
213213
log.Debug("cache: revalidation worker stopped")
@@ -233,9 +233,9 @@ func (s *cacheSpec) lruBytesScraper() {
233233
}
234234

235235
type revalJob struct {
236-
key string
237-
req *http.Request // pre-cloned, safe to use after the originating request ends
238-
doRevalFn func() // closure with access to per-instance doRevalidate
236+
key string
237+
req *http.Request // pre-cloned, safe to use after the originating request ends
238+
filter *cacheFilter // instance whose doRevalidate to call
239239
}
240240

241241
type cacheFilter struct {
@@ -704,11 +704,9 @@ func (f *cacheFilter) Response(ctx filters.FilterContext) {
704704
func (f *cacheFilter) enqueueRevalidation(key string, orig *http.Request) {
705705
cloned := orig.Clone(context.Background())
706706
job := revalJob{
707-
key: key,
708-
req: cloned,
709-
doRevalFn: func() {
710-
f.doRevalidate(key, cloned)
711-
},
707+
key: key,
708+
req: cloned,
709+
filter: f,
712710
}
713711
select {
714712
case f.revalJobs <- job:

filters/cache/filter_test.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ func TestCacheFilter_KeyIsolationByAuthToken(t *testing.T) {
140140
f := fi.(*cacheFilter)
141141
t.Cleanup(f.Close)
142142
t.Cleanup(spec.(*cacheSpec).client.Close)
143+
t.Cleanup(spec.(*cacheSpec).Close)
143144
f.fetch = func(*http.Request) (*http.Response, error) {
144145
return nil, errors.New("no fetch stub set")
145146
}
@@ -264,6 +265,7 @@ func TestCacheFilter_Response_NoopIfStateBagKeyMissing(t *testing.T) {
264265
func TestCreateFilter_InvalidArgs(t *testing.T) {
265266
spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: "localhost:9090", L1TTL: 60 * time.Second})
266267
t.Cleanup(spec.(*cacheSpec).client.Close)
268+
t.Cleanup(spec.(*cacheSpec).Close)
267269
cases := []struct {
268270
name string
269271
args []interface{}
@@ -1319,6 +1321,7 @@ func TestCacheFilter_MustRevalidate_ForcesCoalesceWhenStale(t *testing.T) {
13191321
func TestCacheFilter_SharedStorage_RouteIsolation(t *testing.T) {
13201322
spec := NewCacheFilter(Options{MaxBytes: 1 << 20, ListenAddr: "localhost:9090", L1TTL: 60 * time.Second})
13211323
t.Cleanup(spec.(*cacheSpec).client.Close)
1324+
t.Cleanup(spec.(*cacheSpec).Close)
13221325

13231326
makeFilter := func(t *testing.T) *cacheFilter {
13241327
t.Helper()
@@ -2804,9 +2807,9 @@ func TestCacheFilter_RevalDropped_WhenQueueFull(t *testing.T) {
28042807
return nil, errors.New("blocked fetch")
28052808
}
28062809

2807-
// Send one dummy job so the worker goroutine wakes and blocks inside fetch.
2810+
// Send one job so the worker goroutine wakes and blocks inside fetch.
28082811
dummyReq, _ := http.NewRequest(http.MethodGet, "http://example.com/dummy", nil)
2809-
f.revalJobs <- revalJob{key: "dummy-wake", req: dummyReq}
2812+
f.revalJobs <- revalJob{key: "dummy-wake", req: dummyReq, filter: f}
28102813

28112814
// Wait for the worker to confirm it is inside fetch — no timing guesswork.
28122815
<-workerIn

0 commit comments

Comments
 (0)