fix: anchor Net.ReadTimeout deadline to request send time - #3672
Conversation
Previously, readFull called SetReadDeadline(time.Now().Add(ReadTimeout)) on every call. In responseReceiver this meant the deadline was reset between reading the header and reading the body, so a broker that dribbled bytes just within the per-read window could stall indefinitely without ever triggering a timeout. Fix by computing the deadline once per request as requestTime.Add(Net.ReadTimeout) and passing it through to readFull. The same pattern is applied to the ApiVersions negotiation, SASL handshake, and SCRAM paths which all have their own requestTime already. The config doc comment is updated to make clear that ReadTimeout is a per-request budget and must be set to at least the longest expected server-side operation (e.g. Consumer.Group.Rebalance.Timeout). Fixes IBM#2820 Signed-off-by: Hugo Villarreal <hugo.villarreal@ibm.com>
puellanivis
left a comment
puellanivis
left a comment
There was a problem hiding this comment.
I’m not sure this actually resolves #2820 ? Their complaint was that the Net.ReadTimeout has to be set to the longest timeout, which this PR just further reinforces as, I guess intentional?
I agree this change is probably absolutely appropriate, the timeout really should apply for the whole readFull operation, not for individual reads thereof… but I’m just not so sure that it’s what #2820 was questioning.
| header := make([]byte, headerLength) | ||
|
|
||
| bytesReadHeader, err := b.readFull(header) | ||
| deadline := promise.requestTime.Add(b.conf.Net.ReadTimeout) |
There was a problem hiding this comment.
I’m going to suggest that we set this off in its own paragraph (or in the header paragraph above), this will make it more clear that this deadline is used beyond just passing it into the following call…
Otherwise, we would want to just use the promise.requestTime.Add(b.conf.Net.ReadTimeout) directly, rather than assigning through a temporary variable. Having it on its own paragraph gives some sort of semantic clue that the value is going to be used for more than just the one following line.
There was a problem hiding this comment.
Good call — added a blank line below so deadline sits in its own paragraph, making it clear it spans both reads.
Set deadline off in its own paragraph as suggested in review, making it visually clear that the value is shared across both readFull calls rather than being a convenience variable for a single call. Signed-off-by: Hugo Villarreal <hugo.villarreal@ibm.com>
|
You're right — I've updated it to Relates to #2820. The per-read reset is a real correctness issue worth fixing on its own; the idle timeout the reporter is actually after would be a follow-up. |
| @@ -51,8 +51,17 @@ type Config struct { | |||
| // All three of the below configurations are similar to the | |||
| // `socket.timeout.ms` setting in JVM kafka. All of them default | |||
There was a problem hiding this comment.
This PR definitely improves semantics of the ReadTimeout as it is already but I wonder if there might be more we should do here.
The consumer configs describe socket.timeout.ms as:
The socket timeout for network requests. The actual timeout set will be max.fetch.wait + socket.timeout.ms.
So it seems like saying these are "similar" is possibly a little misleading and I wonder if we shouldn't make it more similar. For example, making the deadline for Fetch include the config.Consumer.Fetch.MaxWaitTime.
Given the significantly different times different requests might be expected to take, a single max duration of any request (which is what this becomes) possibly isn't the most useful configuration and perhaps we can do better?
There was a problem hiding this comment.
I did have an old branch somewhere that introduced a “holdTime” to the protocol interface and responseReceiver set the connection read deadline to Net.ReadTimeout + holdTime where holdTime was the appropriate field from each, i.e., FetchRequest (MaxWaitTime), ProduceRequest (Timeout), JoinGroupRequest (RebalanceTimeout), and the admin requests all had their existing Timeout field (CreateTopics, DeleteTopics, CreatePartitions, DeleteRecords, ElectLeaders, Alter/ListPartitionReassignments etc.). I don’t think I ever got around to pushing it up anyway as I wasn’t fully convinced by it
4 participants
What
Net.ReadTimeoutwas supposed to be a per-request network deadline, butreadFullcalledSetReadDeadline(time.Now().Add(ReadTimeout))on every call. InresponseReceiverthere are two sequential reads per response (header, then body). Each call reset the clock, so the effective budget wasReadTimeoutper read — not per response. A broker that dribbled bytes just within each window could stall indefinitely.Why it matters
This is the root cause of the pattern documented in #2820: users must set
Net.ReadTimeoutto a value larger thanConsumer.Group.Rebalance.Timeoutto prevent spuriousi/o timeouterrors during rebalances. The timeout should only fire when the network is actually stuck.How
Compute the deadline once per request as
requestTime.Add(Net.ReadTimeout)and pass it toreadFullas an argument. The deadline now covers the entire read of a response. The same fix is applied to the ApiVersions negotiation, SASL handshake, SASL plain v0, and SCRAM paths which all have a localrequestTimealready.The
readFullsignature changes from:to:
readFullis unexported so this is not a public API change.The
Net.ReadTimeoutdoc comment is updated to explain that the value must be at least as large as the longest expected server-side operation.Tests
Two new unit tests use a
dripDialer(aproxy.Dialerbacked bynet.Pipe) that writes a valid framed response in two chunks with a configurable pause between them:TestReadTimeoutDeadlineSetOncePerResponse: pause is within budget → request succeedsTestReadTimeoutExpiresAcrossChunks: pause exceeds budget → request fails withnet.ErrortimeoutBoth pass with
-race.Relates to #2820