Skip to content
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

Feature: ContextTimeoutEnabled #520

Open
proost opened this issue Apr 1, 2024 · 1 comment
Open

Feature: ContextTimeoutEnabled #520

proost opened this issue Apr 1, 2024 · 1 comment

Comments

@proost
Copy link
Contributor

proost commented Apr 1, 2024

syncDo and syncDoMulti in pipe can close connection when i/o timeout happens.

when architecture looks like below:

client -- server -- redis

client set timeout to requests. timeout which is set by client can be various(sometime too short) and it can hurt performance on redis and server. because making a new connection is expensive operation both server and redis.

so we need to ContextTimeoutEnabled features like go-redis already did.
If ContextTimeoutEnabled is true, we only respect ConnectionWriteTimeout. Only using ConnectionWriteTimeout, we can satisfy SLO and performance both.

@rueian
Copy link
Collaborator

rueian commented Apr 1, 2024

I don't really like the idea of having a toggle to ignore context timeout globally. Say you first ignore context timeout globally, but later you find you need timeout in some cases, then you have no choice but to flip the toggle again.

Furthermore, syncDo and syncDoMulti could be seldom used if the server serves concurrent requests from clients. Once concurrent requests are detected, rueidis will start pipelining and not use syncDo and syncDoMulti anymore. When pipelining, a connection will not be closed if context timeout is reached.

If the server wants to ignore context timeout, it can pass the context with context.WithoutCancel().
If the server wants to keep connections if timeout, it can set AlwaysPipelining: true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants