Fix request hanging after response start timeout expires#15899
Merged
knative-prow[bot] merged 1 commit intoknative:mainfrom Jun 5, 2025
Merged
Fix request hanging after response start timeout expires#15899knative-prow[bot] merged 1 commit intoknative:mainfrom
knative-prow[bot] merged 1 commit intoknative:mainfrom
Conversation
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.
knative-prow
bot
added
the
size/M
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15899 +/- ##
==========================================
+ Coverage 80.94% 80.97% +0.03%
==========================================
Files 210 210
Lines 16769 16771 +2
==========================================
+ Hits 13573 13580 +7
+ Misses 2844 2837 -7
- Partials 352 354 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
/lgtm |
Contributor
|
/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
Contributor
|
/test unit-tests |
Contributor
|
Captuing the failed unit tests run. It seems related as it's TimeoutTest failure. It might be intermittent though, let see in another run. Or in worse case it's clashing with the other change: https://github.com/knative/serving/pull/15900/files |
Member
|
/lgtm |
norman465
pushed a commit
to src2img/knative-serving
that referenced
this pull request
Jun 17, 2025
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.
SaschaSchwarze0
pushed a commit
to src2img/knative-serving
that referenced
this pull request
Jun 17, 2025
* 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>
isibeni
pushed a commit
to src2img/knative-serving
that referenced
this pull request
Jun 23, 2025
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #15352
Proposed Changes
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.
Release Note