Add DeadlineBody for non-resetting body timeouts#688
Conversation
|
This is just my rambly opinion: I wouldn't expect a body to have a full timeout, which is shared over multiple chunks. Instead, I would expect to set a timeout over some operation consuming the body. Like, if I'm collecting the whole body into a flat It kind of confuses me that the timeout on this body would continue to count even if I wasn't actively polling the body. Like if I polled some of the body, and then started doing something else, and then came back, and it triggers the timeout. And yes, I know there's just such a timeout in reqwest, and I find that equally weird. |
|
@seanmonstar - Yeah, fair, agreed the semantics are a bit unusual if you think of it as a "body timeout" in isolation. The use case I had in mind (and the one in #623) is less "timeout on reading the body" and more "hard cap on how long this request can hold resources." For a public-facing service, you want to bound the total wall-clock time a single connection can occupy a task slot, memory for buffering, etc. A slow client sending one byte per second will never trip an idle timeout, but you still want to evict it eventually. The original issue frames it the same way: "ensure that users don't take up too much of your server's time." You're right that if the consumer stops polling mid-body to go do other work, the clock keeps ticking, which could be surprising. In practice though, the expected pattern is a tight drain loop ( The alternative is wrapping the consume operation in I think there's a third variant that would address the "only count time while actively polling" concern: a cumulative-poll-duration timeout that tracks elapsed time only while a poll is pending and pauses when the consumer isn't polling. That would be useful for cases where you want to enforce minimum client throughput without penalizing server-side processing gaps. It's a meaningfully different semantic though, and probably worth exploring as a separate issue/PR if there's interest. I've actually helped people implement that inside my company. That's a great idea to have and I'd be glad to open an issue for it (and probably we can upstream some of the internal work we've done). This PR targets the simpler wall-clock-deadline case from #623. Happy to add a doc note calling out that the deadline is wall-clock from construction (not cumulative poll time), so the behavior is clear to users. Also open to naming suggestions if |
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub struct AbsoluteTimeoutBody<B> { |
There was a problem hiding this comment.
I wonder, wouldn't it make more sense for this to live in http-body-util?
There was a problem hiding this comment.
Good question, I'd be open to contributing the different types of timeout bodies to that crate, and using them internally in tower-http (since we still do want the service/layer composition helpers).
But, I don't maintain that crate currently and didn't want to add maintainer load, and anyway the TimeoutBody currently lives here.
If you have perms on http-body-util and want to review such a PR, glad to cut one and update this one accordingly :)
There was a problem hiding this comment.
No, I'm not a maintainer on any of the hyperium crates.
|
personally I'd weigh in that this is generally a useful abstraction although one that is often going to have surprising results. It will only be useful for a very narrow range of specific use cases (which isn't inherently on a problem). We have a more sophisticated version of this in the Rust SDK that is basically this, but the timer resets when you get more data (so it is a minimum-speed-limit) basically. This seemed good in practice for being able to to a good job of terminating slow connections but it seems like it mostly causes issues. I would consider naming this |
I don't mind adding in an API knob that has this behavior, seems useful enough, though I don't think it's the right default. Or do you think it is mostly a footgun @rcoh and not worth including? ACK on it being a fairly narrow range of use cases, I can improve docs to be clearer about relevance.
++ i'll change it |
|
@rcoh updated to use Once this lands, I'll cut an issue to |
Closes #623
Summary
Adds
AbsoluteTimeoutBody, a body wrapper that enforces a hard deadline on the entire body transfer. UnlikeTimeoutBody(which resets its timer each time a frame arrives),AbsoluteTimeoutBodystarts a singleSleepat construction and never resets it. If the body isn't fully consumed before the deadline,poll_framereturns aTimeoutError.This is useful for public endpoints where you want to cap total time spent on a request regardless of how frequently data trickles in. A slow client sending one byte per second would never trip
TimeoutBody's idle timeout, but will correctly tripAbsoluteTimeoutBody.I went with a new standalone type rather than a config flag on
TimeoutBody. This keeps the existing API unchanged and avoids branching inTimeoutBody::poll_frame. The correspondingRequestBodyAbsoluteTimeoutandResponseBodyAbsoluteTimeoutservice/layer types mirror the existing idle-timeout equivalents.It's a bit verbose, but I think it's ok. Name bikeshedding is invited by reviewers.
Shares the existing
TimeoutErrortype since the failure mode is the same: body data didn't arrive in time.Testing
Covers the two scenarios that distinguish this from
TimeoutBody:Also includes basic single-frame happy/error path tests, and verifies the error downcasts to
TimeoutError.