Make `PostingsForMatchersCache.PostingsForMatchers` respect context cancellation #551

aknuds1 · 2023-10-24T14:34:05Z

Make PostingsForMatchersCache.PostingsForMatchers respect context cancellation when executing queries (to populate cache), by executing them in a background goroutine taking a dedicated context, which gets canceled once no PostingsForMatchers caller needs it any more.

Fixes #548.

TODO:

Add test exercising concurrent access, where most accesses should use the cache

dimitarvdimitrov

thanks for the work. I'm not super sure about the linked list traversal. I think if we change the linked list traversal, then we'll also take care of the race with the producer.

tsdb/postings_for_matchers_cache.go

dimitarvdimitrov · 2023-10-24T15:02:50Z

tsdb/postings_for_matchers_cache.go

+		c.cachedMtx.Unlock()
+
+		// Wait for query execution goroutine to finish
+		<-promise.done


what's the reason for waiting here?

Maybe it's not necessary any longer, have to review it.

Yeah, it shouldn't be necessary any longer (it was in a previous revision).

Actually, after removing this line I get a data race in tests. Not sure why yet.

One reason to read from promise.done here is that otherwise the background goroutine will block on sending to the channel.

I see. Then we're holding the whole request until the PostingsForMatchers finishes. Can we have a select with default or with case <-ctx.Done() when sending to promise.done? Then we can immediately return the cancelled request and let the background goroutine clean up when it's ready

I think we can solve the blocking on promise.done by moving the call to c.created to the goroutine, and promise.done is used only to indicate completion (see comment below) - then promise.done is only ever closed, and never sent to.

dimitarvdimitrov · 2023-10-24T15:06:22Z

tsdb/postings_for_matchers_cache.go

 		return nil, ctx.Err()
-	case <-p.done:
+	case <-promise.done:
 		// Checking context error is necessary for deterministic tests,
 		// as channel selection order is random
 		if ctx.Err() != nil {
 			return nil, ctx.Err()


nitpick but let's warp the error with something. Chasing plain context canceled isn't fun.

same comment applies above

Done, PTAL. I also added a cause to the cancellation, to ease diagnosis.

dimitarvdimitrov · 2023-10-24T15:10:09Z

tsdb/postings_for_matchers_cache.go

+		for elem := c.cached.Front(); elem != nil; elem = elem.Next() {
+			cc := elem.Value.(*postingsForMatchersCachedCall)
+			if cc.key == key {
+				c.cached.Remove(elem)
+				c.cachedBytes -= cc.sizeBytes
+				break
+			}
+		}


I'm afraid this can be slow. Is there a way to make sure we don't insert it into the linked list if all the contexts are cancelled?

perhaps have a channel to which the producer writes an empty struct when it's done with producing the result. Any consumer will take this empty struct and commit to consuming the result. If there is no consumer waiting on the channel, then the producer just discards the result.

Actually more or less the same idea came to when reviewing the code this morning.

I tried implementing this idea @dimitarvdimitrov, by sending the cache entry size over promise.done for a consumer to pick up and commit, could you have a look?

charleskorn

This is quite a complex change - how necessary is it that we cancel postings for matchers calls if the caller cancels? (How expensive are these calls?) And how often does the caller cancel?

I'm wondering if we can just accept that occasionally we'll waste some cycles completing a postings for matchers call for a caller that has already cancelled.

aknuds1 · 2023-10-25T07:06:50Z

I'm wondering if we can just accept that occasionally we'll waste some cycles completing a postings for matchers call for a caller that has already cancelled.

@charleskorn as discussed offline, I see your concern, and I think we'll need to analyze the necessity of being able to cancel executing queries in the light of past issues at Grafana Labs related to this.

charleskorn

I know this is WIP, but I'd suggest adding some unit tests for the different cases as a next step, to help flush out some of the subtle bugs that might be introduced before we refactor and address other issues we've noticed.

charleskorn · 2023-10-25T23:57:22Z

tsdb/postings_for_matchers_cache.go

+		if sizeBytes > 0 {
+			// Got the promise cache entry's size, store it
+			c.created(key, c.timeNow(), sizeBytes)
+		}


Couldn't the call to c.created be made in the goroutine spawned by PostingsForMatchers? Then done can simply be closed to signal that the promise has completed

charleskorn · 2023-10-26T00:01:00Z

tsdb/postings_for_matchers_cache.go

+	c.calls[key] = promise
+	c.cachedMtx.Unlock()
+
+	go func() {


Should we move this to its own method on postingsForMatchersPromise perhaps? This method is starting to get quite big.

charleskorn · 2023-10-26T00:05:57Z

tsdb/postings_for_matchers_cache.go

+
+		// There are no more waiting goroutines, cancel the promise and remove it from the cache
+		promise.cancel(fmt.Errorf("no remaining callers interested in query"))
+		delete(c.calls, key)


I don't think this is quite right - calling delete here will remove the promise from the cache if a subsequent call's context is cancelled, even if the promise succeeded. Imagine this sequence of events:

Initial call for a set of matchers arrives, promise succeeds and value is cached and returned.

Subsequent call arrives with a context that is already cancelled. Cached promise is retrieved from cache, waiting is incremented to 1 and then waitOnPromise is called.

waitOnPromise observes cancelled context, decrements waiting to 0 and removes cached promise from cache.

Instead, we only want to cancel and remove the call from c.calls if there's nothing waiting and the call is still in progress.

charleskorn · 2023-10-26T00:14:31Z

tsdb/postings_for_matchers_cache.go

+		c.cachedMtx.Unlock()
+
+		// Wait for query execution goroutine to finish
+		<-promise.done


I think we can solve the blocking on promise.done by moving the call to c.created to the goroutine, and promise.done is used only to indicate completion (see comment below) - then promise.done is only ever closed, and never sent to.

…ation Signed-off-by: Arve Knudsen <[email protected]>

Signed-off-by: Arve Knudsen <[email protected]>

pracucci · 2024-07-04T07:58:28Z

Superseded by #644

aknuds1 requested review from charleskorn and dimitarvdimitrov October 24, 2023 14:34

aknuds1 force-pushed the arve/postings-for-matchers-cache-handle-cancellation branch from 3d8194b to 750587f Compare October 24, 2023 14:44

aknuds1 added the bug Something isn't working label Oct 24, 2023

dimitarvdimitrov reviewed Oct 24, 2023

View reviewed changes

charleskorn reviewed Oct 24, 2023

View reviewed changes

aknuds1 requested a review from dimitarvdimitrov October 25, 2023 07:00

charleskorn reviewed Oct 26, 2023

View reviewed changes

aknuds1 added 4 commits November 14, 2023 10:25

PostingsForMatchersCache.PostingsForMatchers: Respect context cancell…

0ecca4a

…ation Signed-off-by: Arve Knudsen <[email protected]>

Make sure to increment promise.waiting

b8d6f1e

Signed-off-by: Arve Knudsen <[email protected]>

Wrap context errors

ebf9cdd

Signed-off-by: Arve Knudsen <[email protected]>

Only commit cache entry after completed query

9ac9d68

Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 force-pushed the arve/postings-for-matchers-cache-handle-cancellation branch from f1040da to 9ac9d68 Compare November 14, 2023 09:26

pracucci closed this Jul 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `PostingsForMatchersCache.PostingsForMatchers` respect context cancellation #551

Make `PostingsForMatchersCache.PostingsForMatchers` respect context cancellation #551

aknuds1 commented Oct 24, 2023 •

edited

Loading

dimitarvdimitrov left a comment

dimitarvdimitrov Oct 24, 2023

aknuds1 Oct 24, 2023

aknuds1 Oct 24, 2023

aknuds1 Oct 24, 2023

aknuds1 Oct 25, 2023 •

edited

Loading

dimitarvdimitrov Oct 25, 2023

charleskorn Oct 26, 2023

dimitarvdimitrov Oct 24, 2023

dimitarvdimitrov Oct 24, 2023

aknuds1 Oct 24, 2023

dimitarvdimitrov Oct 24, 2023

aknuds1 Oct 25, 2023

aknuds1 Oct 25, 2023

charleskorn left a comment

aknuds1 commented Oct 25, 2023 •

edited

Loading

charleskorn left a comment •

edited

Loading

charleskorn Oct 25, 2023

charleskorn Oct 26, 2023

charleskorn Oct 26, 2023

charleskorn Oct 26, 2023

pracucci commented Jul 4, 2024

Make PostingsForMatchersCache.PostingsForMatchers respect context cancellation #551

Make PostingsForMatchersCache.PostingsForMatchers respect context cancellation #551

Conversation

aknuds1 commented Oct 24, 2023 • edited Loading

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleskorn left a comment

Choose a reason for hiding this comment

aknuds1 commented Oct 25, 2023 • edited Loading

charleskorn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci commented Jul 4, 2024

Make `PostingsForMatchersCache.PostingsForMatchers` respect context cancellation #551

Make `PostingsForMatchersCache.PostingsForMatchers` respect context cancellation #551

aknuds1 commented Oct 24, 2023 •

edited

Loading

aknuds1 Oct 25, 2023 •

edited

Loading

aknuds1 commented Oct 25, 2023 •

edited

Loading

charleskorn left a comment •

edited

Loading