warp: fewer timer operations around streaming response bodies - #1098
warp: fewer timer operations around streaming response bodies#1098seanparsons wants to merge 1 commit into
Conversation
sendFragment wrapped every streamed fragment in T.resume before the send and T.pause after it: two timer-queue operations per fragment, per request. Streaming now resumes the handle once before the body, tickles per fragment, and pauses once after. Semantics change, deliberate: the timeout is armed while user code computes between fragments, so a responseStream that produces nothing for a full timeout period is killed (previously it could stall forever between fragments, since the timeout was only armed during warp's own sends). Streams that write at least once per timeout period (or send heartbeats) are unaffected; the sends themselves are now also covered, closing a slowloris-shaped gap. Verified manually that a stalled stream is closed at the configured timeout while an idle keep-alive connection still times out normally. Standalone this halves the per-fragment timer ops (one tickle instead of resume+pause); it compounds with the time-manager tickle rate-limiting change, which makes the per-fragment tickle nearly free. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011HBrMGfWRxTpYeEB4UT8Kf
Vlix
left a comment
Vlix
left a comment
There was a problem hiding this comment.
I don't like the change in behaviour.
We shouldn't act like a stream is a DOS/slow-loris if the user code is taking a long time.
Could this maybe work with a sort of "debounced" resume/pause function?
@seanparsons I don't understand which code this "pauses" refers to. |
Ah yes, it's referring to this wai/warp/Network/Wai/Handler/Warp/HTTP1.hs Line 189 in 8208479 |
|
Thanks. If we were to accept your changes, I think placing |
3 participants
Part of a series splitting #1090 into independently reviewable PRs. Works standalone (halves per-fragment timer ops); compounds with the time-manager tickle rate-limiting PR, which makes the per-fragment tickle nearly free.
sendFragmentwrapped every streamed fragment inT.resume/T.pause: two timer-queue operations per fragment. Streaming now resumes once before the body, tickles per fragment, and pauses once after.Deliberate semantics change for maintainers to weigh: The timeout is armed while user code computes between fragments, so a
responseStreamthat produces nothing for a full timeout period is killed, previously it could stall forever between fragments, since the timeout was only armed during warp's own sends. Streams writing at least once per timeout period (or sending heartbeats) are unaffected, and the sends themselves are now covered, closing a slowloris-shaped gap. Observable for e.g. SSE without heartbeats. Verified manually that a stalled stream closes at the configured timeout.