New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Client gets stuck #322
Comments
Hi @GiedriusS, When rueidis operates in pipelining mode, it actually doesn't use SetDeatline but uses periodic PINGs to detect timeouts: Lines 556 to 560 in d6ca19e
where the Though I am not sure where you were getting stuck now, the line:
looks the most suspicious to me. I will do more investigation on it. |
Hi @GiedriusS, Just noticed that the suspicious line is outdated. Could you try a newer version? |
Let me try updating tomorrow and update you if this reoccurs. |
Hi @GiedriusS, I hope you're doing well. Did the issue reoccur again? After revisiting the old code and the suspicious line you provided, I believe the issue was already fixed by the PR #291, where I changed the previous timeout handling of resp := resultsp.Get(len(multi), len(multi)) // get a resp container
...
abort:
go func(i int) {
for ; i < len(resp.s); i++ { // the resp container might already be recycled, its length has changed
<-ch
}
}(i)
err := newErrResult(ctx.Err())
for ; i < len(resp.s); i++ {
resp.s[i] = err
}
return resp // return the resp container to the upper layer, and the upper layer will recycle it to this resp := resultsp.Get(len(multi), len(multi)) // get a resp container
...
abort:
go func(i int, resp *redisresults) {
<-ch
resultsp.Put(resp) // recycle the old resp container
}(i, resp)
resp = resultsp.Get(len(multi), len(multi)) // get a new resp container
err := newErrResult(ctx.Err())
for ; i < len(resp.s); i++ {
resp.s[i] = err
}
return resp With this change, the cleanup goroutine doesn't depends on |
Update client to fix redis/rueidis#322. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
* go.mod: update Redis client Update client to fix redis/rueidis#322. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> * *: run go mod tidy Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> --------- Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
No, it helped. I also updated the client upstream in thanos-io/thanos#6589. 🙇 thank you for your help |
* go.mod: update Redis client Update client to fix redis/rueidis#322. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> * *: run go mod tidy Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com> --------- Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Somehow during timeouts, the Rueidis client gets stuck.
From /debug/pprof:
Most notably in our setup we are passing
context.Background()
as the context to the Set commands:https://github.com/thanos-io/thanos/blob/2606855c922285649d46ef472b02c6ea20e31fe2/pkg/cacheutil/redis_client.go#L252
But, we set
ConnWriteTimeout
to 3 seconds. Inside of the rueidis client I can find SetDeadline so that should set both write/read timeouts?All in all, it looks like the client depends on the context having a deadline? 🤔 is this correct?
The text was updated successfully, but these errors were encountered: