fix(internal/pool): call SetDeadline even if timeout is zero #2176
Conversation
internal/pool/conn.go
Outdated
| if err := cn.netConn.SetReadDeadline(cn.deadline(ctx, timeout)); err != nil { | ||
| return err | ||
| } | ||
| if err := cn.netConn.SetReadDeadline(cn.deadline(ctx, timeout)); err != nil { |
There was a problem hiding this comment.
I think this should be timeout >= 0 so it works with PubSub
There was a problem hiding this comment.
In the pubsub.ReceiveTimeout(ctx, timeout) case, we don't say anything about the timeout argument, is it intended to ignore negative timeout?
for example pubsub.ReceiveTimeout(ctx, -1) means do not set a deadline, use the previous set deadline ?
_, _ = pubsub.ReceiveTimeout(ctx, 3*time.Second) // this call set the deadline to time.Now().Add(3*time.Second)
time.Sleep(3)
_, _ = pubsub.ReceiveTimeout(ctx, -1) // this call will timeout immediately, but it may be intended to block foreverThere was a problem hiding this comment.
ReceiveTimeout(0) is the same as Receive() which means block forever. Negative values should probably disable Set*Deadline calls completely.
But I admit it is subtle.
options.go
Outdated
| ReadTimeout time.Duration | ||
| // Timeout for socket writes. If reached, commands will fail | ||
| // with a timeout instead of blocking. | ||
| // with a timeout instead of blocking. Use value -1 for no timeout and 0 for default. |
There was a problem hiding this comment.
-1 for no timeout is meant to disable the timeout, but it looks like you always call SetReadDeadline no matter what. Perhaps the comment must be updated?
There was a problem hiding this comment.
This sentence is copied from the ReadTimeout description above.
I think no timeout == disable the timeout == block until read/write succeeds, do I understand this correctly?
-1 will be adjusted to 0 here: https://github.com/go-redis/redis/blob/084c0c8914d4d84918a81b867b61d94b6c343ecd/options.go#L152-L157
then in SetReadDeadline or SetWriteDeadline, the timeout is diabled, and the read/write operation will block until it succeeds.
There was a problem hiding this comment.
That is correct, but it looks like we should also support -2 to disable Set*Deadline calls completely.
|
I can confirm this also fixes the disconnect behaviour when using Redis sentinel. |
3 participants
This PR revert #2060, which caused #2175.
Fixes #2175.