fix: wait for hijacked connections to close during drain#16625
fix: wait for hijacked connections to close during drain#16625immanuwell wants to merge 1 commit into
Conversation
|
Hi @immanuwell. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: immanuwell The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16625 +/- ##
==========================================
+ Coverage 80.28% 80.38% +0.09%
==========================================
Files 217 217
Lines 13568 13589 +21
==========================================
+ Hits 10893 10923 +30
+ Misses 2312 2303 -9
Partials 363 363 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| func (w *hijackTrackerResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { | ||
| conn, rw, err := websocket.HijackIfPossible(w.ResponseWriter) |
There was a problem hiding this comment.
I don't know if we want to introduce websocket hijacking into the queue proxy. The idea before was to track this transparently so we're not pulling in extra dependencies etc.
Fixes #
Proposed Changes
net.ConnclosesFlush()andUnwrap()on the tracker wrapperHijack()path, not just a blocked handlerRepro:
TERMto the queue-proxy while that socket is still open.Hijack(), soHijackedDrainer.Drain()may see zero inflight too early and exit. The websocket gets cut off early, which is kinda rough.Related: follow-up to #16362.
Release Note