-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support for context-aware connection #342
Comments
I plan to use it for handling timeouts when hitting the network with the client (in this case, when using |
I was not aware the connection was closed when a timeout error occurred, but I am seeing that locally now after some testing. Is that intended behavior in this package? Imho it seems a bit much to have to create a new connection each time one of its operations times out. And yes, the snippet you posted above is basically what I had in mind with the code additions. |
I agree - in general it seems like the Redis client shouldn't close by default on cancellations/timeouts, but rather leave that to the calling code to handle the situation as it sees fit. It might be worth it to update the Edit: The backwards compatibility of this change kind of worries me though, especially considering this package has quite a lot of users. |
The The client in the |
What is the problem you are trying to solve? You said that you want handle timeouts. Read and write timeouts can be specified when dialing a connection and the application can override the read timeout on a per-call basis. Explain why this does not meet your needs. It may be that adding a context argument is part of fixing the problem, but let's start with a discussion of the problem. |
The problem I'm trying to solve is cancellation, where timeouts are just a special case. The |
Describe the specific problem you are trying to fix in your application. I know what context does. There's no need to repeat a description here. |
I'm working with a multi-stage pipeline where stage 3 involves using Redis as a cache to detect duplicate values, but is dependent on other stages still running to continue. If another stage fails at some point during runtime, I'd like to be able to initiate early cancellation on all other stages without waiting for each of their long-running tasks to end. |
Redis does not provide a way to cancel a command. Which Redis command is taking so long that it's a concern? Are you using one of the blocking commands? |
No blocking commands, just basic stuff like |
Misclicked the close button when replying, sorry. I am also setting timeouts as we discussed earlier, but currently I can already do that separately by just using the |
Is there a Redis command that you need to cancel? |
@redmondnoodle I do not recommend your proposed |
I am closing this issue because there's not a strong argument for a new feature. Although the application stages may be long running, individual calls to GET and SETEX are not. The application can terminate a stage without involving this package. The dial timeout options handle timeouts for GET and SETEX commands. There's no need to change the API when there are no features to add. |
@garyburd I have this use case: a goroutine is spawned waiting for something to happen by means of a "BRPOP key 0" (0 -- so no timeout). Now, as @smotes mentions above as well, I need to do a graceful shutdown after receiving SIGINT. The usual way of doing this is to use a context. Without having a context in place, how can the goroutine doing the BRPOP be properly signalled to stop what it is doing? |
@garyburd While the DoWithCancel() from #351 could be used to implement my use case, that approach does result in each of my Do() commands to have an extra overhead/latency for the additional Redis commands to obtain the client ID -- something that would not be necessary if there was "native" support for cancelling any ongoing command in a Redis connection. FWIW, I have now implemented my use case by rather bluntly issuing a conn.Close() when the cancel is triggered. This aborts the ongoing Do("BRPOP"), albeit with an error:
|
Extra roundtrips to the server can be eliminated by caching the result of the CLIENT ID command in the low-level connection. I updated 351 to mention this. Native support cannot eliminate the overhead of coordinating with a goroutine to wait on the context done channel. I proposed DoWithCancel() as a separate helper so that applications incur this extra overhead only where needed. |
Similar to the
ConnWithTimeout
interface that implements theDo/Receive
methods with an extratime.Duration
parameter, I think it would be useful to add in aConnWithContext
interface that takes an extracontext.Context
parameter. Many other Golang libraries are starting to adoptcontext.Context
usage for cancellation since it was put into the standard library. Additionally, this proposed change shouldn't make any breaking changes to this package's public API. I am happy to implement this feature if I get the green light from whoever maintains this package - I just wanted to start up a discussion first per https://github.com/gomodule/redigo/blob/master/.github/CONTRIBUTING.md.The text was updated successfully, but these errors were encountered: