Fix race condition in timeout handler causing activator crashes #16222
Conversation
knative-prow
bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Fedosin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fedosin
force-pushed
the
timeout-headers
branch
from
f0debeb to
d270d63
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16222 +/- ##
==========================================
+ Coverage 80.04% 80.10% +0.06%
==========================================
Files 214 214
Lines 13313 13325 +12
==========================================
+ Hits 10656 10674 +18
+ Misses 2296 2292 -4
+ Partials 361 359 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add synchronization for HTTP header map access during request timeouts. When a timeout occurs, capture a snapshot of headers to prevent concurrent modification by the inner handler after the timeout handler returns. Fixes knative#15850
Fedosin
force-pushed
the
timeout-headers
branch
from
d270d63 to
a2e2dde
Compare
| }) | ||
| } | ||
|
|
||
| func TestTimeoutHandlerConcurrentHeaderAccess(t *testing.T) { |
There was a problem hiding this comment.
Are we sure that this test would actually catch verify the problem?
I tried running it locally without the fix 1000 times but the test didn't fail.
| // Capture a snapshot of headers before marking as timed out | ||
| // to prevent concurrent access to the underlying header map | ||
| tw.headers = make(http.Header) | ||
| for k, v := range tw.w.Header() { | ||
| tw.headers[k] = slices.Clone(v) | ||
| } | ||
|
|
||
| tw.w.WriteHeader(http.StatusGatewayTimeout) |
There was a problem hiding this comment.
Not 100% sure if I understood the timeoutHandler/Writer stuff correctly but wouldn't we still see a concurrent access to the map here?
2 participants
Fixes #15850
Proposed Changes
Add synchronization for HTTP header map access during request timeouts. When a timeout occurs, capture a snapshot of headers to prevent concurrent modification by the inner handler after the timeout handler returns.
Release Note