Skip to content

Commit

Permalink
fix(memcached): use default branch avoid writing to closed chan (#7833
Browse files Browse the repository at this point in the history
)

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

**What this PR does / why we need it**:
Follow up to #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 <kavirajkanagaraj@gmail.com>
  • Loading branch information
kavirajk committed Dec 2, 2022
1 parent 37b1c0f commit 25f4dda
Showing 1 changed file with 9 additions and 7 deletions.
16 changes: 9 additions & 7 deletions pkg/storage/chunk/cache/memcached.go
Original file line number Diff line number Diff line change
Expand Up @@ -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++
}
}
Expand All @@ -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
}
}
Expand Down

0 comments on commit 25f4dda

Please sign in to comment.