Correct tryTimeoutAndWriteError to write timeout regardless of prior writes#15900
Conversation
knative-prow
bot
added
the
size/L
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15900 +/- ##
==========================================
- Coverage 80.94% 80.93% -0.01%
==========================================
Files 210 210
Lines 16769 16769
==========================================
- Hits 13573 13572 -1
- Misses 2844 2845 +1
Partials 352 352 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| defer tw.mu.Unlock() | ||
|
|
||
| if tw.lastWriteTime.IsZero() { | ||
| if !tw.timedOut { |
There was a problem hiding this comment.
is this always true when nothing has been written?
Looking at the changes to the comment and the removal of lastWriteTime. (Sounded to me the "has not been written" part was added b/c of the lastWriteTime ?
Just wondering 😅
There was a problem hiding this comment.
Well, there are two situations when we set the timeout flag:
1. When we reach the response start timeout: https://github.com/knative/serving/blob/main/pkg/http/handler/timeout.go#L248
2. When we hit the regular timeout: https://github.com/knative/serving/blob/main/pkg/http/handler/timeout.go#L230
Currently, both functions (tryResponseStartTimeoutAndWriteError and tryTimeoutAndWriteError) are identical and do the following:
Timeout (write an error header and set the timeout flag to true) if nothing has been written to the response.
This means that we never get inside the condition on line 228: either we write something and lastWriteTime.IsZero() is false, or we don’t write anything and fail earlier by response start timeout.
In my vision, tryTimeoutAndWriteError should always write the error, ignoring whether anything was written before or not. Checking the timedOut flag is needed to make this function idempotent.
There was a problem hiding this comment.
@Fedosin thanks for the further explanation. It makes sense to me even though I don't a deep knowledge of Serving's timeout handling.
There was a problem hiding this comment.
In my vision, tryTimeoutAndWriteError should always write the error, ignoring whether anything was written before or not.
That would be unexpected for end-users - so we shouldn't do this.
If something is written on the wire already and a timeout occurs we should just close the connection.
Fedosin
force-pushed
the
tryTimeoutAndWriteError
branch
from
bad6248 to
d2f583e
Compare
…writes Previously, the function comment suggested it would only write errors if nothing had been written, but the implementation correctly only checks the timedOut flag. This allows timeout errors to be written even after a response has started, which is the desired behavior for handling slow responses. - Fixed misleading function comment - Updated test to match actual behavior - Added comprehensive test coverage
Fedosin
force-pushed
the
tryTimeoutAndWriteError
branch
from
d2f583e to
2ce75a8
Compare
|
PTAL |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsimansk, Fedosin, matzew The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
the
approved
|
One odd thing I find weird about this package is if a time out happens - we should exit the for loop and close the connection - but we only do that conditionally for some reason which doesn't seem right. The idle timeout should be the only one that is looping. for {
select {
// ...
case <-timeout.C():
timeoutDrained = true
if tw.tryTimeoutAndWriteError(h.body) {
return
}
}
} |
…writes (knative#15900) Previously, the function comment suggested it would only write errors if nothing had been written, but the implementation correctly only checks the timedOut flag. This allows timeout errors to be written even after a response has started, which is the desired behavior for handling slow responses. - Fixed misleading function comment - Updated test to match actual behavior - Added comprehensive test coverage
* Fix flakes in TestIdleTimeoutHandler (knative#15918) * Correct tryTimeoutAndWriteError to write timeout regardless of prior writes (knative#15900) Previously, the function comment suggested it would only write errors if nothing had been written, but the implementation correctly only checks the timedOut flag. This allows timeout errors to be written even after a response has started, which is the desired behavior for handling slow responses. - Fixed misleading function comment - Updated test to match actual behavior - Added comprehensive test coverage * Fix request hanging after response start timeout expires (knative#15899) When a response starts before the responseStartTimeout but the timeout still fires, the timeout handler would not properly continue processing the request. This caused requests to hang indefinitely if they started responding just before the responseStartTimeout expired. The issue occurred because after handling the responseStartTimeout case, the select loop would continue but without properly waiting for the handler to complete. Setting responseStartTimeoutDrained ensures the timer is properly cleaned up and the loop continues to process other events (completion, overall timeout, or idle timeout). Fixes requests that start responding before responseStartTimeout but take longer than responseStartTimeout to complete. --------- Co-authored-by: Mike Fedosin <mfedosin@redhat.com>
* Fix flakes in TestIdleTimeoutHandler (knative#15918) * Correct tryTimeoutAndWriteError to write timeout regardless of prior writes (knative#15900) Previously, the function comment suggested it would only write errors if nothing had been written, but the implementation correctly only checks the timedOut flag. This allows timeout errors to be written even after a response has started, which is the desired behavior for handling slow responses. - Fixed misleading function comment - Updated test to match actual behavior - Added comprehensive test coverage * Fix request hanging after response start timeout expires (knative#15899) When a response starts before the responseStartTimeout but the timeout still fires, the timeout handler would not properly continue processing the request. This caused requests to hang indefinitely if they started responding just before the responseStartTimeout expired. The issue occurred because after handling the responseStartTimeout case, the select loop would continue but without properly waiting for the handler to complete. Setting responseStartTimeoutDrained ensures the timer is properly cleaned up and the loop continues to process other events (completion, overall timeout, or idle timeout). Fixes requests that start responding before responseStartTimeout but take longer than responseStartTimeout to complete. --------- Co-authored-by: Mike Fedosin <mfedosin@redhat.com>
4 participants
Fixes #15486
Proposed Changes
Previously, the function comment suggested it would only write errors if nothing had been written, but the implementation correctly only checks the timedOut flag. This allows timeout errors to be written even after a response has started, which is the desired behavior for handling slow responses.
Release Note