fix: use inactivity timeout for watch instead of whole-request timeout - #3022
fix: use inactivity timeout for watch instead of whole-request timeout#3022duizabojul wants to merge 1 commit into
Conversation
kubernetes-prow
Bot
added
the
do-not-merge/work-in-progress
|
Welcome @duizabojul! |
kubernetes-prow
Bot
added
the
size/L
|
|
kubernetes-prow
Bot
added
the
cncf-cla: no
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: duizabojul 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 |
2fdfe13 to
b8a873e
Compare
Watch.watch() passed AbortSignal.timeout(requestTimeoutMs) to fetch, which counts from request start and stays armed while the response body streams. Any watch, healthy or not, was therefore aborted with a TimeoutError after 30s. Replace it with a resettable timer on the AbortController: it is armed before the fetch (connect timeout), then re-armed on the 200 response and on every body chunk, so requestTimeoutMs now means "max time without data". The timer is unref'd and cleared when the watch is done. No public API change. Also reset ListWatch's reconnectDelayMs to 0 in the TimeoutError branch of doneHandler. A client-side timeout means we already waited the full request timeout, so reconnecting immediately cannot tight-loop, while backing off would leave quiet resources unwatched for up to MAX_RECONNECT_DELAY_MS. Exponential backoff still applies to real errors and server-side disconnects.
b8a873e to
6086604
Compare
|
/easycla |
kubernetes-prow
Bot
added
cncf-cla: yes
kubernetes-prow
Bot
removed
the
do-not-merge/work-in-progress
|
this looks good to me. this exposed a couple other issues to address with watches like lack of jitter on the reconnects which i'll handle in a separate PR. |
kubernetes-prow
Bot
added
the
lgtm
2 participants
Fixes #3020
What / why
WatchpassesAbortSignal.timeout(requestTimeoutMs)to the fetch that streams the watch body, so the signal fires 30s after the request starts regardless of connection health, and every watch — including one actively receiving events — is aborted with aTimeoutError.Per the discussion in #3020, this PR keeps timeout signaling but changes what it measures:
src/watch.ts— replaces the whole-requestAbortSignal.timeoutwith a resettable inactivity timer. The timer is armed before the fetch (connect timeout, same as today), then re-armed on the 200 response and on every body data chunk. A watch that keeps receiving data is left alone; a connection that goes silent forrequestTimeoutMs(or never connects) is still aborted with the sameTimeoutErroras before. The timer isunref()ed to matchAbortSignal.timeout's event-loop behavior, and is cleared once the watch finishes. No public API change;requestTimeoutMskeeps its name and 30s default, its meaning becomes "max time with no data received".src/cache.ts—ListWatchnow resetsreconnectDelayMsin the existingTimeoutErrorbranch ofdoneHandler. A client-side timeout means the client already waited the fullrequestTimeoutMs, so an immediate reconnect cannot tight-loop — the timeout itself is the throttle. Without this, a watch on a quiet resource (which now times out everyrequestTimeoutMsby design) accumulates exponential backoff and ends up alternating ~30s watching / ~30s not watching. Backoff is unchanged for real errors and server-side disconnects.Tests
requestTimeoutMs, then clean server close →done(null))TimeoutErrorTimeoutErrorreconnects apply no delay, while a subsequent non-timeout reconnect still backs off (via the injectabledelayFn)Both new watch tests and the cache test fail against the previous implementation.
npm test(356/356),npm run lint,npm run buildall pass.