From 25f4dda811279382d96354d0e2522e90b01c9430 Mon Sep 17 00:00:00 2001 From: Kaviraj Kanagaraj Date: Fri, 2 Dec 2022 11:12:53 +0100 Subject: [PATCH] fix(memcached): use `default` branch avoid writing to closed chan (#7833) Signed-off-by: Kaviraj **What this PR does / why we need it**: Follow up to https://github.com/grafana/loki/pull/7817 This PR uses `default` branch instead of `inputCh <- work` to make sure we are writing to closed `inputCh`. Gist is, `default` run [only when none of the branch is ready](https://go.dev/tour/concurrency/6). which makes more sense rather than to have `inputCh <- work` (writing to closed channel on the branch condition) These can be explained by these two tiny snippets. * [with `default`](https://go.dev/play/p/-FspbTZd20I) * [without `default`](https://go.dev/play/p/Ag4WznOaEq0) **Which issue(s) this PR fixes**: Fixes #NA **Special notes for your reviewer**: We already have test `TestMemcached_fetchKeysBatched` to catch this behaviour. In fact this test caught this probabilistic behaviour in some CI failures. [here ](https://drone.grafana.net/grafana/loki/17853/3/4) and[ here](https://drone.grafana.net/grafana/loki/17854/3/4) Thanks @DylanGuedes for catching this CI failures. **Checklist** Signed-off-by: Kaviraj --- pkg/storage/chunk/cache/memcached.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg/storage/chunk/cache/memcached.go b/pkg/storage/chunk/cache/memcached.go index 97ad8b022d..025a46cee0 100644 --- a/pkg/storage/chunk/cache/memcached.go +++ b/pkg/storage/chunk/cache/memcached.go @@ -172,12 +172,13 @@ func (c *Memcached) fetchKeysBatched(ctx context.Context, keys []string) (found select { case <-c.closed: return - case c.inputCh <- &work{ - keys: batchKeys, - ctx: ctx, - resultCh: resultsCh, - batchID: j, - }: + default: + c.inputCh <- &work{ + keys: batchKeys, + ctx: ctx, + resultCh: resultsCh, + batchID: j, + } j++ } } @@ -199,7 +200,8 @@ func (c *Memcached) fetchKeysBatched(ctx context.Context, keys []string) (found select { case <-c.closed: return - case result := <-resultsCh: + default: + result := <-resultsCh results[result.batchID] = result } }