store: fix nil panic in proxy heap #5746
Conversation
With lazy proxying we need to wait for at least one response before we can build a heap properly. Closes thanos-io#5717 Fixes thanos-io#5552. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
| return true | ||
| } | ||
|
|
||
| for len(l.bufferedResponses) == 0 { |
There was a problem hiding this comment.
Do we need to unlock the mutex here before waiting?
There was a problem hiding this comment.
No, Wait() already takes care of everything for us: https://pkg.go.dev/sync#Cond.Wait.
There was a problem hiding this comment.
Thanks for the quick fix. Would #5742 (comment) also get resolved by this?
Yep, I have tested these changes with your PR. It was 100% reproducible before and I couldn't reproduce the nil panic after these changes. |
There was a problem hiding this comment.
The fix is looking good to me, I was just looking at this 👍
One question related with #5717 (comment) though, do we then still need to handle 0 response case in https://github.com/thanos-io/thanos/blob/main/pkg/store/proxy_heap.go#L114? I'd want to avoid returning nil if that's not a valid case if we are not handling it, to make the code more obvious.
Good point. Maybe we could even add a |
Yeah, given that it's really an "invalid case", this solution makes sense to me 👍 |
This is pretty much a bug if this ever happens so complain loudly. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
|
Just ran this also on top #5552, everything is working as expect 👌, nice job! Whoop, just look like one test in the proxy needs adjusting (probably just remove the |
|
This is nice. @GiedriusS We want to tag |
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
This code is not in 0.28.0 and it's under a hidden flag, by the way (: |
* store: fix nil panic in proxy heap With lazy proxying we need to wait for at least one response before we can build a heap properly. Closes thanos-io#5717 Fixes thanos-io#5552. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> * store: panic if At() called without Next() This is pretty much a bug if this ever happens so complain loudly. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> * store: adjust test after recent changes Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> Signed-off-by: utukj <utukphd@gmail.com>
With lazy proxying we need to wait for at least one response before we can build a heap properly.
Closes #5717 Fixes #5552.
Improved e2e tests to cover this case. cc @matej-g @fpetkovski
Signed-off-by: Giedrius Statkevičius giedrius.statkevicius@vinted.com