From 8df8db6c73132c305a7c0818553d1a34faa040e6 Mon Sep 17 00:00:00 2001 From: Charles Korn Date: Fri, 20 Oct 2023 15:40:08 +1100 Subject: [PATCH 1/3] Add failing test Signed-off-by: Charles Korn --- tsdb/postings_for_matchers_cache_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tsdb/postings_for_matchers_cache_test.go b/tsdb/postings_for_matchers_cache_test.go index ff591fe189..d196dcdab8 100644 --- a/tsdb/postings_for_matchers_cache_test.go +++ b/tsdb/postings_for_matchers_cache_test.go @@ -336,6 +336,29 @@ func TestPostingsForMatchersCache(t *testing.T) { require.Equal(t, 1, callsPerMatchers[matchersKey(matchersLists[2])]) require.Equal(t, 1, callsPerMatchers[matchersKey(matchersLists[3])]) }) + + t.Run("initial request context is cancelled, second request is not cancelled", func(t *testing.T) { + matchers := []*labels.Matcher{labels.MustNewMatcher(labels.MatchEqual, "foo", "bar")} + expectedPostings := index.NewListPostings(nil) + + c := newPostingsForMatchersCache(time.Hour, 5, 1000, func(ctx context.Context, ix IndexPostingsReader, ms ...*labels.Matcher) (index.Postings, error) { + if ctx.Err() != nil { + return nil, ctx.Err() + } + + return expectedPostings, nil + }, &timeNowMock{}, false) + + ctx1, cancel := context.WithCancel(context.Background()) + cancel() + _, err := c.PostingsForMatchers(ctx1, indexForPostingsMock{}, true, matchers...) + require.Equal(t, context.Canceled, err) + + ctx2 := context.Background() + actualPostings, err := c.PostingsForMatchers(ctx2, indexForPostingsMock{}, true, matchers...) + require.NoError(t, err) + require.Equal(t, expectedPostings, actualPostings) + }) } func BenchmarkPostingsForMatchersCache(b *testing.B) { From 6dcebc9e25b35d12f460f4c61f2351fef4b9a38e Mon Sep 17 00:00:00 2001 From: Charles Korn Date: Fri, 20 Oct 2023 15:45:36 +1100 Subject: [PATCH 2/3] Don't allow cancelled contexts to poison the postings for matchers cache. Signed-off-by: Charles Korn --- tsdb/postings_for_matchers_cache.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/tsdb/postings_for_matchers_cache.go b/tsdb/postings_for_matchers_cache.go index aea96ca3dc..6b99487710 100644 --- a/tsdb/postings_for_matchers_cache.go +++ b/tsdb/postings_for_matchers_cache.go @@ -78,27 +78,31 @@ func (c *PostingsForMatchersCache) PostingsForMatchers(ctx context.Context, ix I return c.postingsForMatchers(ctx, ix, ms...) } c.expire() - return c.postingsForMatchersPromise(ctx, ix, ms)() + return c.postingsForMatchersPromise(ix, ms)(ctx) } type postingsForMatcherPromise struct { - sync.WaitGroup + done chan struct{} cloner *index.PostingsCloner err error } -func (p *postingsForMatcherPromise) result() (index.Postings, error) { - p.Wait() - if p.err != nil { - return nil, p.err +func (p *postingsForMatcherPromise) result(ctx context.Context) (index.Postings, error) { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-p.done: + if p.err != nil { + return nil, p.err + } + return p.cloner.Clone(), nil } - return p.cloner.Clone(), nil } -func (c *PostingsForMatchersCache) postingsForMatchersPromise(ctx context.Context, ix IndexPostingsReader, ms []*labels.Matcher) func() (index.Postings, error) { +func (c *PostingsForMatchersCache) postingsForMatchersPromise(ix IndexPostingsReader, ms []*labels.Matcher) func(context.Context) (index.Postings, error) { promise := new(postingsForMatcherPromise) - promise.Add(1) + promise.done = make(chan struct{}) key := matchersKey(ms) oldPromise, loaded := c.calls.LoadOrStore(key, promise) @@ -106,9 +110,10 @@ func (c *PostingsForMatchersCache) postingsForMatchersPromise(ctx context.Contex promise = oldPromise.(*postingsForMatcherPromise) return promise.result } - defer promise.Done() + defer close(promise.done) - if postings, err := c.postingsForMatchers(ctx, ix, ms...); err != nil { + // FIXME: do we need to cancel the call to postingsForMatchers if all the callers waiting for the result have cancelled their context? + if postings, err := c.postingsForMatchers(context.Background(), ix, ms...); err != nil { promise.err = err } else { promise.cloner = index.NewPostingsCloner(postings) From efcd876b501a70152811039e454ec5600fecd42d Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Fri, 20 Oct 2023 16:16:10 +0200 Subject: [PATCH 3/3] Ensure deterministic execution, for tests Signed-off-by: Arve Knudsen --- tsdb/postings_for_matchers_cache.go | 17 ++++++++++++++--- tsdb/postings_for_matchers_cache_test.go | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/tsdb/postings_for_matchers_cache.go b/tsdb/postings_for_matchers_cache.go index 6b99487710..865224f7f3 100644 --- a/tsdb/postings_for_matchers_cache.go +++ b/tsdb/postings_for_matchers_cache.go @@ -93,6 +93,11 @@ func (p *postingsForMatcherPromise) result(ctx context.Context) (index.Postings, case <-ctx.Done(): return nil, ctx.Err() case <-p.done: + // Checking context error is necessary for deterministic tests, + // as channel selection order is random + if ctx.Err() != nil { + return nil, ctx.Err() + } if p.err != nil { return nil, p.err } @@ -107,12 +112,18 @@ func (c *PostingsForMatchersCache) postingsForMatchersPromise(ix IndexPostingsRe key := matchersKey(ms) oldPromise, loaded := c.calls.LoadOrStore(key, promise) if loaded { - promise = oldPromise.(*postingsForMatcherPromise) - return promise.result + // promise was not stored, we return a previously stored promise, that's possibly being fulfilled in another goroutine + close(promise.done) + return oldPromise.(*postingsForMatcherPromise).result } + + // promise was stored, close its channel after fulfilment defer close(promise.done) - // FIXME: do we need to cancel the call to postingsForMatchers if all the callers waiting for the result have cancelled their context? + // Don't let context cancellation fail the promise, since it may be used by multiple goroutines, each with + // its own context. Also, keep the call independent of this particular context, since the promise will be reused. + // FIXME: do we need to cancel the call to postingsForMatchers if all the callers waiting for the result have + // cancelled their context? if postings, err := c.postingsForMatchers(context.Background(), ix, ms...); err != nil { promise.err = err } else { diff --git a/tsdb/postings_for_matchers_cache_test.go b/tsdb/postings_for_matchers_cache_test.go index d196dcdab8..41ccaacd47 100644 --- a/tsdb/postings_for_matchers_cache_test.go +++ b/tsdb/postings_for_matchers_cache_test.go @@ -352,7 +352,7 @@ func TestPostingsForMatchersCache(t *testing.T) { ctx1, cancel := context.WithCancel(context.Background()) cancel() _, err := c.PostingsForMatchers(ctx1, indexForPostingsMock{}, true, matchers...) - require.Equal(t, context.Canceled, err) + require.ErrorIs(t, err, context.Canceled) ctx2 := context.Background() actualPostings, err := c.PostingsForMatchers(ctx2, indexForPostingsMock{}, true, matchers...)